-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve precompiles #20793
improve precompiles #20793
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the performance of the usual using DataFrames
tests (from an empty lib code cache) after this change?
src/rtutils.c
Outdated
n += jl_printf(out, "."); | ||
if (!hidden) { | ||
n += jl_printf(out, "."); | ||
if (globfunc && !jl_id_start_char(u8_nextchar(sn,&i))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing spaces after a handful of the ,
's in here
src/rtutils.c
Outdated
@@ -526,11 +528,44 @@ static size_t jl_static_show_x_(JL_STREAM *out, jl_value_t *v, jl_datatype_t *vt | |||
} | |||
else if (vt == jl_datatype_type) { | |||
jl_datatype_t *dv = (jl_datatype_t*)v; | |||
if (dv->name->module != jl_core_module) { | |||
jl_sym_t *globname = dv->name->mt != NULL ? dv->name->mt->name : NULL; | |||
int globfunc = globname && jl_get_global(dv->name->module, globname) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jl_get_global
can mutate m
& print deprecation warnings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jl_get_global
can also allocate?
else if (globfunc) { | ||
n += jl_printf(out, "typeof("); | ||
} | ||
if (dv->name->module != jl_core_module || !jl_module_exports_p(jl_core_module, sym)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jl_module_exports_p
can allocate and can't be used here (or should be fixed not to allocate?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can fix it not to allocate.
I enabled |
FWIW, the REPL feels very snappy indeed! |
292f474
to
56f59d7
Compare
Comments addressed. I manually removed the closures. Hopefully there will still be a bit of improvement. |
56f59d7
to
0179f27
Compare
Bump. @JeffBezanson, we you able to determine what the performance is of |
src/rtutils.c
Outdated
n += jl_printf(out, ":%s", jl_symbol_name((jl_sym_t*)v)); | ||
char *sn = jl_symbol_name((jl_sym_t*)v); | ||
// TODO check for valid identifier | ||
int quoted = strchr(sn,'/') && strcmp(sn,"/") && strcmp(sn,"//") && strcmp(sn,"//="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing spaces after the ,
's here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... this isn't in the formatting guidelines though.
I tried but I haven't been able to get DataFrames to load on master yet. |
Ah, yeah... Try checking out DataArrays to master then checking out DataFrames to anj/06. |
Ok, with this change precompiling DataFrames goes from 44 seconds to 50 seconds. Does that mean too many precompiles or not enough? |
Is this a recent regression? The only acceptable answer here is < 20s. |
Adding to milestone since updating precompiles should go with the release process, and also to remind us to look at the DataFrames compile time regression. |
As a quick aside regarding DataFrames, it's now installable directly from |
DataFrames compile time regression persists. |
Looking at |
- improve static_show to readably print a much larger set of types - add TRACE_COMPILE option to print `precompile` calls
0179f27
to
f794796
Compare
Ok, I updated this to just improve the mechanism. Package load times can be improved a bit by adding precompile statements, and there might be a couple percent slowdown from #20343, but we still have a large performance gap vs. 0.5. |
OK, thanks. I reviewed the biggest contributors to inference time increase and think I can start working on a patch. That doesn't need to hold this up. It would be good to make sure the new precompile file includes whatever entries that |
CI failures are known issues |
Symbol("text/plain")
, etc.precompile
function actually ignores to avoid poisoning the cache).The system image is a bit bigger after this, but the repl is really snappy.