[BEAM-37] DoFnReflector: Add invoker interface and generate code #521
Conversation
|
@lukecwik @kennknowles Probably not ready for a full review yet, but please take a look in case there is any high-level feedback. |
| @@ -429,6 +429,12 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>com.google.code.findbugs</groupId> | |||
There was a problem hiding this comment.
If you remove the above jsr305 dependency, you can probably get rid of the above ignoredUnusedDeclaredDependency config. I have this fix in a branch that isn't yet ready to merge-- perhaps I should get that in.
There was a problem hiding this comment.
Is it now in? I am confused by this since we use findbugs annotations all over the place prior to this PR.
There was a problem hiding this comment.
Yes, my findbugs PR is now in, which switches all dependencies to use com.google.code.findbugs:annotations instead of jsr305.
If you still get unused/undeclared dependency errors after switching, run mvn dependecy:tree to see if jsr305 is being brought in transitively; you may need to explicitly exclude it.
|
Not a big fan of how prepareForProcessing is wired through via isStartBundle flag but can't come up with a cleaner proposal since I think creating a bunch of specialized subclasses will just make it worse then having the one flag. Otherwise I'm happy with the approach, adding any additional methods / changing method invocation should be pretty easy. |
|
Typo in the PR title comes from a typo in the commit subject line. |
| @@ -449,6 +455,12 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>net.bytebuddy</groupId> | |||
| <artifactId>byte-buddy</artifactId> | |||
| <version>1.4.3</version> | |||
There was a problem hiding this comment.
I think this is a candidate for versioning in the <dependencyManagement> section.
|
This is awesome. It is a lot to chew on. I need to learn more about the meaning of various bytebuddy interfaces. Given the tests, I am pretty happy. I'd love even more tests, given the potential fragility and debugging challenges of dropping to bytecode. I will think a bit about what they might be. |
bjchambers
force-pushed
the
byte-buddy-dofn
branch
from
009be34
to
b419bb0
Compare
bjchambers
changed the title
| <!-- This dependency is only used for an annotation which doesn't | ||
| show up in bytecode, so doesn't show up as used. --> | ||
| <ignoredUnusedDeclaredDependencies> | ||
| <ignoredUnusedDeclaredDependency>com.google.code.findbugs:findbugs-annotations</ignoredUnusedDeclaredDependency> |
There was a problem hiding this comment.
The actual fix here is, I think, to exclude jsr305 which presumably was pulled in by bytebuddy. Otherwise everyone downstream has trouble with the dependency analysis as well.
There was a problem hiding this comment.
(but you should just check the dependency tree to see why this is happening)
The method to call for a DoFnWithContext requires reflection since the shape of the parameters may change. Doing so in each processElement call puts this refelection in the hot path. This PR introduces a DoFnInvoker interface which is bound to a specific DoFnWithContext and delegates the three important methods (startBundle, processElement, finishBundle). It uses byte-buddy to generate a simple trampoline implementation of the DoFnInvoker class for each type of DoFnWithContext. This leads to 2-3x better performance in micro-benchmarks of method dispatching.
bjchambers
force-pushed
the
byte-buddy-dofn
branch
from
54308ee
to
8a43437
Compare
|
LGTM. Might need build unbreakage from @swegner. |
|
Merging... |
|
Typically when we change
@bjchambers : Did you have that discussion off-thread and, if not, can we verify that we're doing the right thing in all places? |
* chore: remove googleapis/firestore-dpe from codeowners * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.
The method to call for a DoFnWithContext requires reflection since the
shape of the parameters may change. Doing so in each processElement call
puts this refelection in the hot path.
This PR introduces a DoFnInvoker interface which is bound to a specific
DoFnWithContext and delegates the three important methods (startBundle,
processElement, finishBundle).
It uses byte-buddy to generate a simple trampoline implementation of
the DoFnInvoker class for each type of DoFnWithContext.
This leads to 2-3x better performance in micro-benchmarks of method
dispatching.