[
https://issues.apache.org/jira/browse/CALCITE-3786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17139390#comment-17139390
]
Vladimir Sitnikov commented on CALCITE-3786:
--------------------------------------------
[~julianhyde], I agree that we should not nitpick the fine details within
benchmarks. I agree we should not raise the bar again and again.
However, all the comments are not something advanced. They are very basic
requests. I just ask to make the benchmark to measure the thing the patch
touches. I don't even ask to implement multiple different benchmarks.
Performance in this PR is very important (even in the case being able to
customize digest is important as well), so I would expect that there should be
reasonable tests before the PR is merged.
--
The current benchmark measures the wrong thing: it uses the same object
instances to lookup in the cache. In typical rule execution, one creates new
(!) rels with a relbuilder, then they register the result in the planner.
---
I could expect the following benchmarks:
1) Overall execution of the test suite (e.g. capture timings from slow_tests in
CI). Note: the current PR seems to perform early normalization of RexNodes,
which alone might shift the response time significantly.
2) Planner#registerRel execution time. For instance: create a planner, register
same rels there. Then the benchmark would create new (!) rels and call
planner.register (or whatever it is named). That benchmark code would be the
same in both Calcite versions (original and patched). The measurement values
should be "response time", and "allocation rate".
3) Digest computation time. This might be something like "create rel and
compute its digest". The measurement values should be "response time", and
"allocation rate". This might be interesting for fine-tuning digset
implementation (e.g. to eliminate excessive array allocations), however, digest
is cached, so "digest computation" benchmark alone provides very little
information on the impact of the fix on the overall performance.
4) Memory footprint. It is probably to be measured with Java Object Layout. For
instance, we create a relnode instance, and weight the object graph.
The benchmark in https://github.com/apache/calcite/pull/2034 looks like #3,
however, there are major flaws.
----
{quote} I think we now need to stop asking him to jump over higher and higher
hurdles{quote}
It is sad to receive "-1 because it results in lots of irrelevant plan changes"
when I suggest "early RexCall digest normalization". That is exactly "asking to
jump over higher and higher hurdles". I had to invent and implement a more
complicated approach.
It is really sad to see that the very same contributor commits the patch that
performs that early normalization (with all the "irrelevant plan changes") with
not a single blink.
Note: I do not complain that the PR removes "my" code. I complain that there's
no acknowledgment that my original proposal (normalize RexCall in the
constructor) was viable from the very beginning.
---
I have no time to work on Calcite for the following 14 days, and I have no time
to write benchmarks or even analyze the results.
> Add Digest interface to enable efficient hashCode(equals) for RexNode and
> RelNode
> ---------------------------------------------------------------------------------
>
> Key: CALCITE-3786
> URL: https://issues.apache.org/jira/browse/CALCITE-3786
> Project: Calcite
> Issue Type: New Feature
> Components: core
> Affects Versions: 1.21.0
> Reporter: Vladimir Sitnikov
> Assignee: Danny Chen
> Priority: Major
> Fix For: 1.24.0
>
> Time Spent: 6h 20m
> Remaining Estimate: 0h
>
> Current digests for RexNode, RelNode, RelType, and similar cases use String
> concatenation.
> It is easy to implement, however, it has drawbacks:
> 1) String objects cannot be reused. For instance, RexCall has operands,
> however, the digest is duplicated. It causes extra memory use and extra CPU
> for string copying
> 2) There's no way to have multiple #toString() methods. RelType might need
> multiple digests: "including field names", "excluding field names".
> A suggested resolution might be behind the lines of
> {code:java}
> class Digest { // immutable
> final int hashCode; // speedup hashCode and equals
> final Object[] contents; // The values are either other Digest objects or
> Strings
> String toString(); // e.g. for debugging purposes
> int compareTo(Digest); // e.g. for debugging purposes.
> }
> {code}
> Note how fields in Kotlin are aligned much better, and it makes it easier to
> read:
> {code:java}
> class Digest { // immutable
> val hashCode: Int // speedup hashCode and equals
> val contents: Array<Any> // The values are either other Digest objects or
> Strings
> fun toString(): String // e.g. for debugging purposes
> fun compareTo(other: Digest): Int // e.g. for debugging purposes.
> }
> {code}
> Then the digest for RexCall could be the bits relevant to RexCall itself +
> digests of the operands (which can be reused as is)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
