Skip to content
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

Canonicalize names of nested functions by keeping a more fine grained counter -- per (module, method name) pair #53719

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

d-netto
Copy link
Member

@d-netto d-netto commented Mar 13, 2024

As mentioned in #53716, we've been noticing that precompile statements lists from one version of our codebase often don't apply cleanly in a slightly different version.

That's because a lot of nested and anonymous function names have a global numeric suffix which is incremented every time a new name is generated, and these numeric suffixes are not very stable across codebase changes.

To solve this, this PR makes the numeric suffixes a bit more fine grained: every pair of (module, top-level/outermost function name) will have its own counter, which should make nested function names a bit more stable across different versions.

This PR applies @JeffBezanson's idea of making the symbol name changes directly in current-julia-module-counter.

Here is an example:

julia> function foo(x)
           function bar(y)
               return x + y
           end
       end
foo (generic function with 1 method)

julia> f = foo(42)
(::var"#bar#foo##0"{Int64}) (generic function with 1 method)

@d-netto
Copy link
Member Author

d-netto commented Mar 13, 2024

Serialization doesn't seem happy with this PR...

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to keep this mostly inside flisp (particularly parsed_method_stack). I also don't think the module name has much value in being mangled into it. Making the name be a combination of the containing method + index into that method name seems somewhat prettier, I also don't see how that canonicalizes names any more than before. I don't really like that this adds more hidden and secret global statefulness to the lowering algorithm as well.

@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch 5 times, most recently from fd607bd to 5fa2de9 Compare March 20, 2024 01:40
@kpamnany
Copy link
Contributor

kpamnany commented Mar 20, 2024

I think you need to keep this mostly inside flisp (particularly parsed_method_stack). I also don't think the module name has much value in being mangled into it. Making the name be a combination of the containing method + index into that method name seems somewhat prettier, I also don't see how that canonicalizes names any more than before. I don't really like that this adds more hidden and secret global statefulness to the lowering algorithm as well.

@vtjnash: to reiterate, our goal here is to improve the consistency of the names generated for these functions. Currently, adding one lambda inside one function can change the generated names of all nested functions across all modules in our application, which means that a number of the precompile statements emitted via --trace-compile by the previous version will fail on the new version.

This PR essentially makes the generated names/numbers independent of changes to other modules+functions.

Do you see any other way for us to accomplish this or does this explanation address your objection(s)?

@d-netto
Copy link
Member Author

d-netto commented Mar 20, 2024

across all modules

Wrong since the counter is already per-module (grep for fl_current_module_counter).

Still, as mentioned above, the proposed change would ensure that adding a nested function doesn't perturb the name generation in literally every numeric suffixed name in a given module.

@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch 6 times, most recently from e76200d to f053663 Compare March 22, 2024 01:23
@kpamnany
Copy link
Contributor

We've tested this change by exercising it through compilation warmup in our application. Without this, we saw roughly 53% warmup effectiveness when upgrading versions. With this, we're seeing 68% warmup effectiveness. Although this comparison is not quite apples-to-apples, there is a notable reduction in precompile evaluation failures across versions and warmed up performance has improved. So this is a good change for us.

@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch from f053663 to 024678d Compare March 22, 2024 22:44
@kpamnany
Copy link
Contributor

Ran more tests to verify the gains from this and have verified that this is a positive change for us.

Any objections to merging @JeffBezanson?

src/ast.c Outdated Show resolved Hide resolved
@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch 2 times, most recently from 756775b to f7a24bb Compare March 27, 2024 14:28
src/ast.c Outdated Show resolved Hide resolved
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @vtjnash that I don't like how this creates an implicit state internal to ast.c that has to be considered together with the program flow through the flisp for correct-ness.

Is it not possible instead to pass a relevant stack of "parent" functions through cl-convert and then into current-julia-module-counter, so that all of this is handled explicitly in the flisp?

src/ast.c Outdated Show resolved Hide resolved
src/ast.c Outdated Show resolved Hide resolved
src/ast.c Outdated Show resolved Hide resolved
src/datatype.c Outdated Show resolved Hide resolved
@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch from f7a24bb to ab5635a Compare April 1, 2024 15:57
src/datatype.c Outdated Show resolved Hide resolved
@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch 3 times, most recently from 5681df7 to e0474e4 Compare April 1, 2024 18:28
d-netto and others added 29 commits June 7, 2024 14:18
…onfn_typename

Co-authored-by: Cody Tapscott <84105208+topolarity@users.noreply.github.com>
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
@d-netto d-netto force-pushed the dcn-canonicalize-callable-names branch from ba77b2e to bfe5c20 Compare June 7, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants