Add Support for IonElement #69
Conversation
popematt
left a comment
popematt
left a comment
There was a problem hiding this comment.
I have a few questions and suggestions, but overall this is looking pretty good.
| ``` | ||
| ion-java-benchmark read --api dom \ | ||
| --api ion_element_dom \ | ||
| example.10n | ||
| ``` |
There was a problem hiding this comment.
Just curious—when you run this, what sort of results to you get?
There was a problem hiding this comment.
It runs two APIs in sequence, then gives two stacked statistical summaries at the end, one for each API.
There was a problem hiding this comment.
Sorry, I should have been more clear. Approximately how much of a performance difference are you getting between the two APIs when you run this command?
There was a problem hiding this comment.
Approximately a 30-ish percentage improvement in primary_score. While this command is a rough comparison between two APIs, the result aligns well with our more extensive experiments run in SampleTime mode across three ion datasets, where we saw a ~37.09% improvement in primary_score.
src/com/amazon/ion/benchmark/CborJacksonMeasurableReadTask.java
Outdated
Show resolved
Hide resolved
src/com/amazon/ion/benchmark/JsonJacksonMeasurableReadTask.java
Outdated
Show resolved
Hide resolved
| * IonElement API. The "loader" is defined as any context that is tied to a single stream. | ||
| * @throws IOException if thrown during reading. | ||
| */ | ||
| abstract void fullyReadElementFromBuffer(SideEffectConsumer consumer) throws IOException; |
There was a problem hiding this comment.
Not a blocker—it would be nice if we could add new APIs to the benchmark CLI without having to add more methods for each one. (Especially since they are unlikely to be applicable for all data formats.) Have you thought about any ways this could be factored to make it more easily extensible?
There was a problem hiding this comment.
You're right, the current approach is a straightforward one but doesn't scale well. A capability-based system where each format declares which APIs it supports might be better. I'd be happy to refactor this in a follow-up if it's worth doing.
Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>
Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>
Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>
Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>
tgregg
left a comment
tgregg
left a comment
There was a problem hiding this comment.
It's looking good! Note the failing CI build.
Co-authored-by: Tyler Gregg <greggt@amazon.com>
Co-authored-by: Tyler Gregg <greggt@amazon.com>
Co-authored-by: Tyler Gregg <greggt@amazon.com>
3 participants
Summary
This PR adds support for the
ION_ELEMENT_DOMAPI to the ion-java-benchmark-cli, enabling benchmarking of the IonElement library from ion-element-kotlin alongside the existing STREAMING and DOM APIs.Changes
API.java)ION_ELEMENT_DOMenum value with documentationIonMeasurableReadTask.java)IonMeasurableWriteTask.java)generateWriteInstructionsElement()methodMeasurableReadTask.java&MeasurableWriteTask.java)ION_ELEMENT_DOMrouting ingetTask()andsetUpTrial()methodsMain.java)--apioption to includeion_element_domwith format restrictionspom.xml)OptionsTest.java)Performance Analysis
Evaluated on three Ion corpus datasets, including both Ion text and Ion binary data, IonElement shows an average of ~37.09% increase in
primary_scoreof performance compared to IonValue.