mbutrovich opened a new pull request, #1029:
URL: https://github.com/apache/datafusion-comet/pull/1029
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases. You can
link an issue to this PR using the GitHub syntax. For example `Closes #123`
indicates that this PR will close issue #123.
-->
Partially addresses #1024.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in
the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your
changes and offer better suggestions for fixes.
-->
Comet uses JNI jstrings as the keys to updating metrics values on the Spark
side during execution. As described in #1024, currently Comet allocates a
jstring for every metric for every invocation of metrics updating. The calls to
`jni_NewStringUTF` account for over 1% of the on-CPU time in TPC-H SF10 for me.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is
sometimes worth providing a summary of the individual changes in this PR.
-->
Added a HashMap that maps the native string to a jstring to use in JNI
calls. This has the benefit of being many-to-one, whereby multiple nodes with
the same metric name will benefit from the cached jstring. This cache is
populated on demand: if the entry isn't present, we allocate a jstring and
insert it into the cache.
I have some thoughts about this approach that I would love for reviewers to
comment on:
1. I wanted to populate the cache in advance, maybe do a plan traversal when
it's generated in `ExecutePlan`. However, DF's metrics are Options and don't
actually appear to be there until the plan starts executing.
2. What is the thread safety of this approach? It's unclear to me if
multiple threads could be sharing this call stack and trying to write new
values into the cache at the same time. I could wrap this in a mutex in
exchange for a performance hit, but would like to understand if this is even
possible.
3. I think I understood the [jni crate's docs with respect to
GlobalRef](https://docs.rs/jni/latest/jni/objects/struct.GlobalRef.html), but a
sanity check on if this approach could hold references longer than we want (and
leak) would be helpful.
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are
they covered by existing tests)?
-->
Existing tests on the JVM side that exercise metrics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
