-
-
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
internals: better representation for code #31191
Conversation
I think |
4e57aa5
to
a6635a1
Compare
a6635a1
to
e85ab94
Compare
I think |
I like CodeInstance |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
Alright, I admit I cheated a little bit to make the nanosoldier results looks pretty nice. Anyways, I'm pretty happy with this chunk of work now. There's still many improvements that can be made (such as correctness of |
125ca2c
to
0aaec61
Compare
base/errorshow.jl
Outdated
print(iob, " (method too new to be called from this world context.)") | ||
elseif ex.world > max_world(method) | ||
print(iob, " (method deleted before this world age.)") |
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.
I don't quite understand what changed to make this obsolete.
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.
Methods don't have world-ages bounds. They were never supposed to, and it doesn't make any logical sense to claim to have them.
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.
Then what does the Method.max_world
field mean?
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.
Doesn't mean anything—that's why it's not correct to have and is now removed
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.
Does that mean that the world
keyword argument to hasmethod
should be deprecated (in 2.0)?
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.
No, while Methods don't have any particularly meaningful world age, the lookup algorithm for a method does
{ | ||
JL_TIMING(INFERENCE); | ||
if (jl_typeinf_func == NULL) | ||
return NULL; | ||
if (jl_is_method(mi->def.method) && mi->def.method->unspecialized == mi) |
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.
If nospecialize is set on all arguments, will we still get a MethodInstance here to infer? It is useful to at least resolve things like loops and literals.
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.
That's a nice feature that this work will allow us to add. Since we don't have it now, I'm not concerned about it right now.
I see about a 10% performance regression in building the system image. Is that expected or am I doing something wrong? |
Might be about right, as some correct computations are much more expensive than we had been doing. Would be helpful to know if you get some performance marks on what the hotspots are. |
I sometimes see ~10% normal variance (or even just NFC PRs like #31306 seeming to show several percent variance), but does seem possibly consistent. The PR:
master branch point:
|
15b80a0
to
cebc5fd
Compare
@JeffBezanson OK to merge? There's additional work I'd like to get started on that builds on this |
This'll help solve many of the problems with edges getting mis-represented and broken by adding an extra level of indirection between MethodInstance (now really just representing a particular specialization of a method) and the executable object, now called Lambda (representing some functional operator that converts some input arguments to some output values, with whatever metadata is convenient to contain there). This fixes many of the previous representation problems with back-edges, since a MethodInstance (like Method) no longer tries to also represent a computation. That task is now relegated strictly to Lambda.
cebc5fd
to
a1a3558
Compare
adapt Cassette for JuliaLang/julia#31191
|
||
if (internal == 1) { | ||
mi->uninferred = jl_deserialize_value(s, &mi->uninferred); | ||
jl_gc_wb(mi, mi->uninferred); |
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.
To me it looks like the order here was (and remains til this day) reversed from serialization: compare
Lines 726 to 736 in c3235cd
write_uint8(s->s, internal); | |
if (!internal) { | |
// also flag this in the backref table as special | |
uintptr_t *bp = (uintptr_t*)ptrhash_bp(&backref_table, v); | |
assert(*bp != (uintptr_t)HT_NOTFOUND); | |
*bp |= 1; | |
} | |
if (internal == 1) | |
jl_serialize_value(s, (jl_value_t*)mi->uninferred); | |
jl_serialize_value(s, (jl_value_t*)mi->specTypes); | |
jl_serialize_value(s, mi->def.value); |
Lines 1611 to 1627 in c3235cd
int internal = read_uint8(s->s); | |
mi->specTypes = (jl_value_t*)jl_deserialize_value(s, (jl_value_t**)&mi->specTypes); | |
jl_gc_wb(mi, mi->specTypes); | |
mi->def.value = jl_deserialize_value(s, &mi->def.value); | |
jl_gc_wb(mi, mi->def.value); | |
if (!internal) { | |
assert(loc != NULL && loc != HT_NOTFOUND); | |
arraylist_push(&flagref_list, loc); | |
arraylist_push(&flagref_list, (void*)pos); | |
return (jl_value_t*)mi; | |
} | |
if (internal == 1) { | |
mi->uninferred = jl_deserialize_value(s, &mi->uninferred); | |
jl_gc_wb(mi, mi->uninferred); | |
} |
If this is indeed a bug, I can submit a fix.
I had thought it might be nice to combine Lambda into MethodInstance, but it turned out to be really awkward since they need to be constructed at different times. We want MethodInstance to be fairly unique on the specialization-tuple (since it's our key for looking up various properties). But we may also need to store several copies of the code properties specialized at various times on assorted dimensions of different inferred properties and world-applicability bounds. That meant we were forced to mutate things at various points in time, and it was really easy for things to become inconsistent. I think it'll be conceptually simpler that each type does a bit less.
I even feel much better about describing this in the dev docs (still to do).This'll help solve many of the problems with edges getting
mis-represented and broken by adding an extra level of indirection between
MethodInstance (now really just representing a particular specialization of a
method) and the executable object, now called Lambda (representing some
functional operator that converts some input arguments to some output
values, with whatever metadata is convenient to contain there).
This fixes many of the previous representation problems with back-edges,
since a MethodInstance (like Method) no longer tries to also represent a
computation. That task is now relegated strictly to Lambda.