Revert storage of method instance in LineInfoNode #50546
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
|
I guess some of the tests need to commented out in the meantime |
|
LGTM (with tests changed to broken=true) |
45 tasks
|
I can confirm that cherry-picking this change resolves the compilation time issue I reported. |
giordano
reviewed
Jul 23, 2023
Comment on lines
+96
to
+102
| @test frame.linfo.def.module === which(func, (Any,)).module broken=true | ||
| @test frame.linfo.def === which(func, (Any,)) broken=true | ||
| @test frame.linfo.specTypes === Tuple{typeof(func), Int} broken=true | ||
| else | ||
| @test frame.linfo.def.module === which(func, (Any,)).module | ||
| @test frame.linfo.def === which(func, (Any,)) | ||
| @test frame.linfo.specTypes === Tuple{typeof(func), Int} |
There was a problem hiding this comment.
I'm late to the party, but this could have been simply
@test frame.linfo.def.module === which(func, (Any,)).module broken=inlined
@test frame.linfo.def === which(func, (Any,)) broken=inlined
@test frame.linfo.specTypes === Tuple{typeof(func), Int} broken=inlinedThe whole point of broken=... is to avoid these if blocks 🙂
KristofferC
pushed a commit
that referenced
this pull request
Jul 24, 2023
KristofferC
added a commit
that referenced
this pull request
Jul 24, 2023
Backported PRs: - [x] #50411 <!-- Fix weird dispatch of * with zero arguments --> - [x] #50202 <!-- Remove dynamic dispatch from _wait/wait2 --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50026 <!-- Store heapsnapshot files in tempdir() instead of current directory --> - [x] #50402 <!-- Add CPU feature helper function --> - [x] #50387 <!-- update newpages pointer after actually sweeping pages --> - [x] #50424 <!-- avoid potential type-instability in _replace_(str, ...) --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50466 <!-- relax assertion involving pg->nold to reflect that it may be a bit in… --> - [x] #50490 <!-- Fix compat annotation for italic printstyled --> - [x] #50488 <!-- fix typo in `Base.isassigned` with `Tridiagonal` --> - [x] #50476 <!-- Profile: Add specifying dir for `take_heap_snapshot` and handling if current dir is unwritable --> - [x] #50461 <!-- fix typo in the --gcthreads argument description --> - [x] #50528 <!-- ssair: Correctly handle stmt insertion at end of basic block --> - [x] #50533 <!-- ensure internal_obj_base_ptr checks whether objects past freelist pointer are in freelist --> - [x] #49322 <!-- improve cat design / performance --> - [x] #50540 <!-- gc: remove over-eager assertion --> - [x] #50542 <!-- gf: remove unnecessary assert cycle==depth --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #50058 <!-- Add unwrapping mechanism for triangular mul and solves --> - [x] #50551 <!-- typeintersect: also record chained `innervars` --> - [x] #50552 <!-- read(io, Char): fix read with too many leading ones --> - [x] #50541 <!-- precompile: ensure globals are not accidentally created where disallowed --> - [x] #50576 <!-- use atomic compare exchange when setting the GC mark-bit --> - [x] #50578 <!-- gf: make method overwrite/delete an error during precompile --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50597 <!-- Fix memory corruption if task is launched inside finalizer --> - [x] #50591 <!-- build: fix various makefile bugs --> - [x] #50599 <!-- faster invalid object lookup in conservative gc --> - [x] #50634 <!-- 🤖 [master] Bump the SparseArrays stdlib from b4b0e72 to 99c99b4 --> - [x] #50639 <!-- Backport LLVM patches to fix various issues. --> - [x] #50546 <!-- Revert storage of method instance in LineInfoNode --> - [x] #50631 <!-- Shift DCE pass to optimize imaging mode code better --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50587 <!-- isassigned for ranges with BigInt indices --> - [x] #50144 <!-- Page based heap size heuristics --> Need manual backport: - [ ] #50595 <!-- Rename ENV variable `JULIA_USE_NEW_PARSER` -> `JULIA_USE_FLISP_PARSER` --> Non-merged PRs with backport label: - [ ] #50637 <!-- Remove SparseArrays legacy code --> - [ ] #50618 <!-- inference: continue const-prop' when concrete-eval returns non-inlineable --> - [ ] #50598 <!-- only limit types in stack traces in the REPL --> - [ ] #50594 <!-- Disallow non-index Integer types in isassigned --> - [ ] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [ ] #50523 <!-- Avoid generic call in most cases for getproperty --> - [ ] #50172 <!-- print feature flags used for matching pkgimage -->
KristofferC
removed
the
backport 1.10
vtjnash
pushed a commit
that referenced
this pull request
Sep 28, 2023
The work I did in #41099 introduced code which, if method information could not be found for an inlined frame, would fall back to use the module of the next-higher stack frame. This often worked there because the only frames that would not be assigned a module at this point would be e.g., `macro expansion` frames. However, due to the performance impact of the way method roots are currently encoded, the extra method roots were removed in #50546. The result is that inlined frames were being assigned a potentially incorrect module, rather than being left blank. Example: ``` julia> @Btime plot([1 2 3], seriestype = :blah) ... [13] #invokelatest#2 @ BenchmarkTools ./essentials.jl:901 [inlined] [14] invokelatest @ BenchmarkTools ./essentials.jl:896 [inlined] ... ```
KristofferC
pushed a commit
that referenced
this pull request
Oct 3, 2023
The work I did in #41099 introduced code which, if method information could not be found for an inlined frame, would fall back to use the module of the next-higher stack frame. This often worked there because the only frames that would not be assigned a module at this point would be e.g., `macro expansion` frames. However, due to the performance impact of the way method roots are currently encoded, the extra method roots were removed in #50546. The result is that inlined frames were being assigned a potentially incorrect module, rather than being left blank. Example: ``` julia> @Btime plot([1 2 3], seriestype = :blah) ... [13] #invokelatest#2 @ BenchmarkTools ./essentials.jl:901 [inlined] [14] invokelatest @ BenchmarkTools ./essentials.jl:896 [inlined] ... ``` (cherry picked from commit ed891d6)
nalimilan
pushed a commit
that referenced
this pull request
Nov 5, 2023
The work I did in #41099 introduced code which, if method information could not be found for an inlined frame, would fall back to use the module of the next-higher stack frame. This often worked there because the only frames that would not be assigned a module at this point would be e.g., `macro expansion` frames. However, due to the performance impact of the way method roots are currently encoded, the extra method roots were removed in #50546. The result is that inlined frames were being assigned a potentially incorrect module, rather than being left blank. Example: ``` julia> @Btime plot([1 2 3], seriestype = :blah) ... [13] #invokelatest#2 @ BenchmarkTools ./essentials.jl:901 [inlined] [14] invokelatest @ BenchmarkTools ./essentials.jl:896 [inlined] ... ``` (cherry picked from commit ed891d6)
aviatesk
pushed a commit
that referenced
this pull request
Jan 14, 2024
The fallback code that was written for #41099 is causing unintended issues with some inlined stack frames (one previous #51405, new #52709), since the main piece, linetable storage and lookup, was removed in #50546. Probably better to strip it all back to how it was previously, until it can all be revisited more fully. Should be backported to 1.10.
aviatesk
pushed a commit
that referenced
this pull request
Jan 14, 2024
The fallback code that was written for #41099 is causing unintended issues with some inlined stack frames (one previous #51405, new #52709), since the main piece, linetable storage and lookup, was removed in #50546. Probably better to strip it all back to how it was previously, until it can all be revisited more fully. Should be backported to 1.10.
Drvi
pushed a commit
to RelationalAI/julia
that referenced
this pull request
Jun 7, 2024
The fallback code that was written for JuliaLang#41099 is causing unintended issues with some inlined stack frames (one previous JuliaLang#51405, new JuliaLang#52709), since the main piece, linetable storage and lookup, was removed in JuliaLang#50546. Probably better to strip it all back to how it was previously, until it can all be revisited more fully. Should be backported to 1.10.
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.
Due to #50082, reverting the causative portion from #41099, which stored MethodInstances in LineInfoNodes.
One possible solution is documented in #50204
Please tag for 1.10 Backport