simpler incremental better backedges #22340
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
src/dump.c
Outdated
| arraylist_push(to_restore, (void*)pcallee); | ||
| arraylist_push(to_restore, (void*)callee); | ||
| *pcallee = (jl_array_t*) HT_NOTFOUND; | ||
| jl_array_ptr_1d_append(callees, callee); |
There was a problem hiding this comment.
callee sounds singular to me, so it's a bit confusing that it's an array. Could use a comment explaining what it's an array of, and/or a variable rename. Is it an array of things called by caller?
|
(backport branch created at jn/release0.6-incremental-better-backedges) |
11a984b to
64b7c46
Compare
yuyichao
reviewed
Jun 14, 2017
src/dump.c
Outdated
| @@ -2175,12 +2175,14 @@ static void jl_insert_methods(jl_array_t *list) | |||
| static size_t lowerbound_dependent_world_set(size_t world, arraylist_t *dependent_worlds) | |||
| { | |||
| size_t i, l = dependent_worlds->len; | |||
| if (world <= (size_t)dependent_worlds->items[l - 1]) | |||
| return world; | |||
| for (i = 0; i < l; i++) { | |||
There was a problem hiding this comment.
So this is sorted and can't be empty? Then only check the first element?
There was a problem hiding this comment.
The value of the return value is also meaningful. (This is doing std::lower_bound on a sorted vector, not just a range test)
There was a problem hiding this comment.
OK. I think I got the sorting order wrong. Though in that case this should probably be a bisect?
There was a problem hiding this comment.
yes, but it wouldn't be faster, so no
the previous attempt to preserve the backedge map was being too cute it is more reliable to simply flatten the whole map into the new caller also corrects the validation that the backedge is not already invalid make sure internal backedges are only internal: this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete that any backedges encountered there are purely internal / new enable GC slightly sooner when loading precompiled modules (part of #20671) reverts 11a984b - but who needed that anyways
d55ec2c to
0e19311
Compare
vtjnash
added a commit
that referenced
this pull request
Jun 15, 2017
the previous attempt to preserve the backedge map was being too cute it is more reliable to simply flatten the whole map into the new caller also corrects the validation that the backedge is not already invalid make sure internal backedges are only internal: this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete that any backedges encountered there are purely internal / new enable GC slightly sooner when loading precompiled modules (part of #20671) reverts 11a984b - but who needed that anyways (cherry-picked from 6fdf36e, PR #22340)
vtjnash
added a commit
that referenced
this pull request
Jun 15, 2017
the previous attempt to preserve the backedge map was being too cute it is more reliable to simply flatten the whole map into the new caller also corrects the validation that the backedge is not already invalid make sure internal backedges are only internal: this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete that any backedges encountered there are purely internal / new enable GC slightly sooner when loading precompiled modules (part of #20671) reverts 11a984b - but who needed that anyways (cherry-picked from 0e19311, PR #22340)
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This uses a much simpler method of restoring backedges, so that we don't have the troublesome edges cases exposed by the old method.
fix #22158
fix #22125