Speed up (pre)compile and load times #681

jmert · 2020-09-20T20:43:22Z

This PR does two primary things with the goal of reducing load time:

The three global Dicts that are rehashed on init are eliminated, replaced with if-else chains at their use or in new internal helper functions. This was motivated by looking at the output of @snoopi using HDF5 (where I started Julia with julia --compiled-modules=no) and noticed that many of the methods near the end with largest times were Dict methods.

One minor advantage of doing this is that property names can now be valid for multiple HDF5 classes. For instance, the __init__ code had to call h5p_set_char_encoding for ASCII_ATTRIBUTE_PROPERTIES and UTF8_ATTRIBUTE_PROPERTIES because :char_encoding was already mapped to H5P_LINK_CREATE so couldn't be used for these two properties which are H5P_ATTRIBUTE_CREATE properties. After replacing the Dict lookup, though, the _prop_set! function can have a :char_encoding case for H5P_ATTRIBUTE_CREATE without any conflicts.

To evaluate the effectiveness of this change, I've timed loading (using HDF5) with a missing precompiled cache, loading with a valid precompiled cache, and the time to run the entire test suite (with the precompiled cache active). The times comparing current master versus the first commit from this PR (run with near-latest Julia, average and std over 10 runs) show:

 master precompile: 3.145 ± 0.0295
no dict precompile: 1.927 ± 0.0164

 master pkg load: 0.706 ± 0.0084
no dict pkg load: 0.564 ± 0.0158

 master pkg test: 32.016 ± 0.9132
no dict pkg test: 31.306 ± 0.4119

The second commit enables a lower optimization level for the entire module in Julia 1.5+. The motivation here is that a lot of functions do not type infer, so we might as well tell the compiler to not try too hard.

Running the same timing tests again:

 master precompile: 3.145 ± 0.0295
no dict precompile: 1.927 ± 0.0164
low opt precompile: 1.860 ± 0.0156

 master pkg load: 0.706 ± 0.0084
no dict pkg load: 0.564 ± 0.0158
low opt pkg load: 0.502 ± 0.0123

 master pkg test: 32.016 ± 0.9132
no dict pkg test: 31.306 ± 0.4119
low opt pkg test: 27.085 ± 0.4015

I've noticed at least one case where type inference improves despite the lower optimization level: getindex(::Union{HDF5File, HDF5Group}, ::String) used to infer as Any but with this PR is inferred returning an HDF5Object.
My hacked together timing script can be found in this gist

jmert · 2020-09-20T20:49:12Z

src/HDF5.jl

+            native_size === Csize_t(2) ? (return Int16) :
+            native_size === Csize_t(4) ? (return Int32) :
+            native_size === Csize_t(8) ? (return Int64) :
+            throw(KeyError(class_id, is_signed, native_size))


I'm not sure this line (and similar in the other branches) is actually necessary — the final check size-8 case in principle could just be the else-case of the size-4 ternary. Thoughts?

musm · 2020-09-20T22:41:24Z

src/HDF5.jl

-    (H5T_FLOAT, nothing, Csize_t(4)) => Float32,
-    (H5T_FLOAT, nothing, Csize_t(8)) => Float64,
-)
+function _hdf5_type_map(class_id, is_signed, native_size)


musm · 2020-09-20T22:44:31Z

src/HDF5.jl

-    fun, propids = hdf5_obj_open[objtype]
-    props = [p_create(prop; pv...) for prop in propids]
-    obj = fun(parent, path, props...)
+    if objtype == H5I_DATASET


👍
I also bet this is just simply also far faster

musm · 2020-09-20T22:45:10Z

src/HDF5.jl

-    UTF8_ATTRIBUTE_PROPERTIES[] = p_create(H5P_ATTRIBUTE_CREATE)
-    h5p_set_char_encoding(UTF8_ATTRIBUTE_PROPERTIES[].id, H5T_CSET_UTF8)
-
-    rehash!(hdf5_type_map, length(hdf5_type_map.keys))


yay, glad we can get rid of this now

musm · 2020-09-20T22:48:45Z

I've noticed at least one case where type inference improves despite the lower optimization level: getindex(::Union{HDF5File, HDF5Group}, ::String) used to infer as Any but with this PR is inferred returning an HDF5Object.

It might then just be worth it to add type annotations where necessary to help the compiler with inference in these cases, instead of (or in addition to) @eval Base.Experimental.@optlevel 1 .

I kind of think adding the line

    @eval Base.Experimental.@optlevel 1

Is not that elegant, given it is current experimental status, however the precompilation latency reduction (10% faster) and improved inference are very welcome.

Overall, very nice PR. Still reviewing some final changes, but 👍

jmert · 2020-09-20T23:04:47Z

I've noticed at least one case where type inference improves despite the lower optimization level: getindex(::Union{HDF5File, HDF5Group}, ::String) used to infer as Any but with this PR is inferred returning an HDF5Object.

It might then just be worth it to add type annotations where necessary to help the compiler with inference in these cases, instead of (or in addition to) @eval Base.Experimental.@optlevel 1 .

Yeah, doing some more systematic review of types and inference's understanding of the code is something worth doing but wasn't as easy of low-hanging fruit. And just because I was maybe a bit unclear, the inference change of getindex is due to rewriting it to not use a dictionary and is independent of the @optlevel 1 addition (i.e. we'll get it even if the second commit is dropped).

musm · 2020-09-21T01:55:15Z

src/HDF5.jl


 a_create(parent::Union{HDF5File, HDF5Object}, name::String, dtype::HDF5Datatype, dspace::HDF5Dataspace) = HDF5Attribute(h5a_create(checkvalid(parent).id, name, dtype.id, dspace.id), file(parent))
+
+function _prop_set!(p::HDF5Properties, name::Symbol, val, check::Bool = true)


Is this check arg really needed, It seems a bit superfluous? I think the rationale is that we can squeeze a bit of perf without having to error check in internal use, but perhaps the compiler is good enough that we don't need such fine grained tuning.

Perhaps these should all be written like (not sure if that helps the compiler guess the right branch?)

if class == H5P_FILE_CREATE elseif class == H5P_FILE_ACCESS elseif class H5P_GROUP_CREATE .... else error(.... end

OTOH the way to code is written as is, is pretty clear.

EDIT:

Actually NVM I don't think it's that feasible given the way the code is structured as is, and would remove some important error checking in the if-clauses.

Is this check arg really needed, It seems a bit superfluous?

The reason I added it was to maintain the current behavior — p_create does not error even if you pass it properties that are not appropriate for a particular property class, while setindex! did (by way of just throwing when the underlying h5p_set_* function errored). The two functions now set false and true, respectively, to maintain that.

src/HDF5.jl

Co-authored-by: Mustafa M <[email protected]>

musm · 2020-09-21T17:45:47Z

Awesome !

musm · 2020-09-21T17:47:20Z

I kinda wonder if we should just disable the integration tests, it's pretty annoying to see red X on PRs that pass.

jmert requested a review from musm September 20, 2020 20:43

jmert commented Sep 20, 2020

View reviewed changes

musm reviewed Sep 20, 2020

View reviewed changes

musm reviewed Sep 21, 2020

View reviewed changes

musm approved these changes Sep 21, 2020

View reviewed changes

musm reviewed Sep 21, 2020

View reviewed changes

src/HDF5.jl Outdated Show resolved Hide resolved

Replace global dicts with if-else chains

ca2c2dd

jmert force-pushed the no_dicts branch from d4d345e to ca2c2dd Compare September 21, 2020 15:07

jmert mentioned this pull request Sep 21, 2020

Use -O1 optimization level #682

Open

musm reviewed Sep 21, 2020

View reviewed changes

src/HDF5.jl Outdated Show resolved Hide resolved

Review comment

7ba3682

Co-authored-by: Mustafa M <[email protected]>

musm merged commit d1e542a into JuliaIO:master Sep 21, 2020

jmert deleted the no_dicts branch September 21, 2020 17:46

jmert mentioned this pull request Sep 30, 2020

Define getindex for properties and rearrange low-level API helpers #685

Merged

3 tasks


		a_create(parent::Union{HDF5File, HDF5Object}, name::String, dtype::HDF5Datatype, dspace::HDF5Dataspace) = HDF5Attribute(h5a_create(checkvalid(parent).id, name, dtype.id, dspace.id), file(parent))

		function _prop_set!(p::HDF5Properties, name::Symbol, val, check::Bool = true)

Uh oh!

Speed up (pre)compile and load times #681

Speed up (pre)compile and load times #681

Uh oh!

Conversation

jmert commented Sep 20, 2020

Uh oh!

jmert Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

musm Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

musm Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musm Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

musm commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmert commented Sep 20, 2020

Uh oh!

musm Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musm Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmert Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

musm commented Sep 21, 2020

Uh oh!

musm commented Sep 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

musm Sep 20, 2020 •

edited

Loading

musm commented Sep 20, 2020 •

edited

Loading

musm Sep 21, 2020 •

edited

Loading

musm Sep 21, 2020 •

edited

Loading