Skip to content

Do more aggressive lambda lifting#1886

Open
OlivierNicole wants to merge 7 commits into
ocsigen:masterfrom
OlivierNicole:more-lambda-lifting
Open

Do more aggressive lambda lifting#1886
OlivierNicole wants to merge 7 commits into
ocsigen:masterfrom
OlivierNicole:more-lambda-lifting

Conversation

@OlivierNicole

@OlivierNicole OlivierNicole commented Mar 14, 2025

Copy link
Copy Markdown
Contributor

With @vouillon we realized that the Lambda_lifting_simple pass that is performed by double translation makes some programs significantly faster. We measured roughly a 1.45 speedup on a large (proprietary) Bonsai benchmark. Presumably V8 is much faster with more toplevel functions and less nested closures.

Making this a draft PR for now because:

  • It should probably be behind a flag: it makes the generated Javascript further from the source code structure;
  • It theoretically results in a quadratic code size.

We probably need to do some measurements before merging. (And maybe with other engines.)

@vouillon vouillon force-pushed the more-lambda-lifting branch 2 times, most recently from 83fff63 to 418e0ed Compare March 21, 2025 14:20
@vouillon vouillon force-pushed the more-lambda-lifting branch from 580b603 to e76cef3 Compare April 7, 2025 13:28
@vouillon vouillon force-pushed the more-lambda-lifting branch from b59bd3e to 25929ef Compare April 7, 2025 22:10
@OlivierNicole

OlivierNicole commented Apr 8, 2025

Copy link
Copy Markdown
Contributor Author

This seems to result in a modest speedup on average, including on partial_render_table. Is this the version of the benchmark that has a memory leak?

@vouillon

vouillon commented Apr 8, 2025

Copy link
Copy Markdown
Member

This seems to result in a modest speedup on average, including on partial_render_table. Is this the version of the benchmark that has a memory leak?

No, I'm using a workaround.

@vouillon vouillon force-pushed the more-lambda-lifting branch from 25929ef to 9e363d1 Compare April 18, 2025 08:57
@OlivierNicole

OlivierNicole commented Apr 28, 2025

Copy link
Copy Markdown
Contributor Author

Should I guard this by an --enable lambda-lift-all option?

@vouillon

vouillon commented Apr 28, 2025

Copy link
Copy Markdown
Member

Yes, you should add a flag.

@OlivierNicole OlivierNicole force-pushed the more-lambda-lifting branch 3 times, most recently from 3f94285 to 49cde7c Compare April 30, 2025 12:26
@OlivierNicole

OlivierNicole commented Apr 30, 2025

Copy link
Copy Markdown
Contributor Author

I re-ran the CI a few times to double-check whether this new pass increases the compilation time. The compilation time seems to oscillate around ±2 %, so I would say it’s in the noise.

@hhugo hhugo force-pushed the more-lambda-lifting branch from 49cde7c to 65cf622 Compare May 9, 2025 08:10
@hhugo

hhugo commented May 21, 2025

Copy link
Copy Markdown
Member

You should probably rebase and rerun the bench now that #1935 is merged to check that this PR still improves perfs

@OlivierNicole OlivierNicole force-pushed the more-lambda-lifting branch from 65cf622 to 0561523 Compare May 21, 2025 13:40
@hhugo

hhugo commented Jun 13, 2025

Copy link
Copy Markdown
Member

@OlivierNicole, there might be some improvement and the size increase seems small. Do you want to rebase the PR one more time and have the feature under a flag ? That way, we can merge instead of letting the PR rot.

@OlivierNicole

OlivierNicole commented Jun 13, 2025

Copy link
Copy Markdown
Contributor Author

Yes, I intend to do this; I just haven’t found the time to squeeze it between my other tasks yet…

@OlivierNicole OlivierNicole force-pushed the more-lambda-lifting branch 2 times, most recently from 0fa87ae to 77b8921 Compare August 25, 2025 13:58
@OlivierNicole

OlivierNicole commented Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

The test failures seem unrelated to this PR, as they also show up on master.

@OlivierNicole OlivierNicole marked this pull request as ready for review August 26, 2025 15:16
@OlivierNicole

OlivierNicole commented Aug 27, 2025

Copy link
Copy Markdown
Contributor Author

Sorry for the delay in coming back to this. I have put the feature behind a flag.

hhugo
hhugo reviewed Sep 1, 2025
View reviewed changes
Comment thread compiler/bin-js_of_ocaml/cmd_arg.ml Outdated
@hhugo

hhugo commented Sep 1, 2025

Copy link
Copy Markdown
Member

Please a small test for this, probably in compiler/tests-compiler/

@OlivierNicole

OlivierNicole commented Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Done. The CI failure seems unrelated.

@OlivierNicole @hhugo
Do more aggressive lambda lifting
4d5114e 
With @vouillon we realized that the `Lambda_lifting_simple` pass that is
performed by double translation makes some programs significantly
faster. We measured roughly a 1.45 speedup on a large (proprietary)
Bonsai benchmark. Presumably V8 is much faster with more toplevel
functions and less nested closures.
@OlivierNicole @hhugo
Make aggressive lambda lifting opt-in
a946c97
@OlivierNicole @hhugo
Update changelog
26f0cbf
@OlivierNicole @hhugo
Harmonize command flag with the existing ones
f7b28cd
@hhugo hhugo force-pushed the more-lambda-lifting branch from e3c5496 to a5f3560 Compare July 11, 2026 06:58
OlivierNicole and others added 3 commits July 11, 2026 10:09
@OlivierNicole @hhugo
Add test for --enable lambda-lift-all
d67e0c0
@hhugo @claude
Add failing regression test: lifted closure calling a free variable
114d9d4 
With --enable lambda-lift-all, the driver runs Lambda_lifting_simple
before Specialize.f, which still uses the Global_flow info computed on
the pre-lifting program. The lifting pass forks the free variables of
lifted closures, so an inexact call to such a variable makes
Global_flow.function_arity index out of bounds and crashes the
compiler:

    Invalid_argument("index out of bounds")
    Raised at Code.Var.Tbl.get (code.ml:272)
    Called from Global_flow.function_arity (global_flow.ml:819)
    Called from Driver.effects_and_exact_calls (driver.ml:206)
    Called from Specialize.specialize_apply ...

The test encodes the intended behavior (the program compiles and
prints len=3) and fails until the driver is fixed, e.g. by lifting
after the flow-info consumers as in the CPS branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hhugo @claude
Fix lambda-lift-all crash: lift after the flow-info consumers
b2ac6ad 
Lambda_lifting_simple forks the free variables of lifted closures, so
the Global_flow info computed before lifting does not cover the
lifted program: Specialize.f would call Global_flow.function_arity on
a forked variable and crash with Invalid_argument("index out of
bounds").

Run the lifting at the end of the effects-disabled branch instead,
after Specialize.f and collects_shapes, mirroring how the CPS branch
lambda-lifts only after Effects.f. This also lets specialization see
the richer pre-lifting flow information.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hhugo hhugo force-pushed the more-lambda-lifting branch from a5f3560 to b2ac6ad Compare July 11, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hhugo hhugo hhugo left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@OlivierNicole @vouillon @hhugo