Simplify compiled page projection #26531
Conversation
raunaqmorarka
requested review from
Copilot,
dain,
electrum,
losipiuk,
lukasz-stec and
piotrrzysko
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the compiled page projection by avoiding the construction of new PageProjectionWork objects per page. Instead of using a factory method handle to create instances, it now reuses a single PageProjectionWork instance and passes parameters directly to the process method.
- Modified PageProjectionWork interface to accept parameters directly in the process method
- Changed GeneratedPageProjection to store and reuse a single PageProjectionWork instance
- Updated the code generation to pass block fields and parameters directly rather than storing them as instance fields
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PageProjectionWork.java | Updated interface to accept parameters directly in process method |
| PageFunctionCompiler.java | Modified code generation to create reusable PageProjectionWork instances and pass parameters directly |
| GeneratedPageProjection.java | Simplified to store and reuse a single PageProjectionWork instance instead of using factory method handles |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/trino-main/src/main/java/io/trino/sql/gen/PageFunctionCompiler.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/gen/PageFunctionCompiler.java
Show resolved
Hide resolved
|
Do you have benchmarks? |
I'm not expected much gain, its mainly to simplify the code |
|
raunaqmorarka
force-pushed
the
page-compiler-simplify
branch
from
ba284a6 to
b10fc06
Compare
Seems the max parameters allowed came down slightly just because of using a method parameter instead of class field to pass an argument. Fixed that now by avoiding that change |
raunaqmorarka
force-pushed
the
page-compiler-simplify
branch
from
b10fc06 to
a1e71f1
Compare
core/trino-main/src/main/java/io/trino/sql/gen/PageProjectionWork.java
Outdated
Show resolved
Hide resolved
raunaqmorarka
force-pushed
the
page-compiler-simplify
branch
from
a1e71f1 to
4fce846
Compare
Avoid constructing new PageProjectionWork object per page
raunaqmorarka
force-pushed
the
page-compiler-simplify
branch
from
4fce846 to
a8093ff
Compare
| Variable to = scope.declareVariable("to", body, add(thisVariable.getField(selectedPositions).invoke("getOffset", int.class), thisVariable.getField(selectedPositions).invoke("size", int.class))); | ||
| for (int i = 0; i < inputChannels.size(); i++) { | ||
| int channel = inputChannels.get(i); | ||
| body.append(thisVariable.setField(blockFields.get(i), page.invoke("getBlock", Block.class, constantInt(channel)))); |
There was a problem hiding this comment.
Looks like those fields are never cleared, so between executions, the last used blocks will be referenced here and thus non-eligible for GC
There was a problem hiding this comment.
right, i was thinking that the PageProjection instance itself would go out of scope once the operator is done.
but on 2nd thought, operators can be blocked, how about we add a clear method to PageProjectionWork which nullifies the blocks ?
3 participants
Description
Avoid constructing new PageProjectionWork object per page
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: