[DO-NOT-MERGE][SPARK-27127][SQL] Deduplicate codes reading from/writing to unsafe object #24050
Conversation
|
Given this is a kind of refactor, I've put this into It would add couple of static method calls: I feel they don't contribute performance hit even they're in critical path, but I could run some benchmarks and update here if we have any. #24016 is a sibling refactor PR, and I'll merge into one if we think that's helpful. |
* UnsafeHelper now only handles binary things
|
The reason of increasing length in code diff is adding license header to two files, as well as representation of method parameters. Not sure how we apply indent on method parameters for Java. |
| */ | ||
| public void writeToMemory(Object target, long targetOffset) { | ||
| Platform.copyMemory(base, offset, target, targetOffset, numBytes); | ||
| UnsafeHelper.writeToMemory(base, offset, target, targetOffset, numBytes); |
There was a problem hiding this comment.
This is just for consistency: writeTo will leverage UnsafeHelper.writeToMemory instead of this method, so it would be safer to make them be same.
| public Decimal getDecimal(int ordinal, int precision, int scale) { | ||
| if (isNullAt(ordinal)) return null; | ||
| if (precision <= Decimal.MAX_LONG_DIGITS()) { | ||
| return Decimal.apply(getLong(ordinal), precision, scale); |
There was a problem hiding this comment.
Other classes use return Decimal.createUnsafe(getLong(ordinal), precision, scale); for same condition. Maybe this is just a missing spot, or some reason to not apply createUnsafe?
|
I think we should better file a JIRA .. |
|
Test build #103305 has finished for PR 24050 at commit
|
|
Test build #103310 has finished for PR 24050 at commit
|
HeartSaVioR
changed the title
|
@HyukjinKwon Thanks for suggestion! Just filed an issue and updated the title. |
|
Test build #103312 has finished for PR 24050 at commit
|
|
cc. @cloud-fan @maropu |
|
I'm worried about refactoring performance critical code like this. A lot of efforts are needed to prove there is no performance regression. |
|
Ah yes. I totally understood. Thanks for providing your voice and sorry for not bringing some numbers to back up. I'll try to have some experiment when I get time (may take some days since I'm dealing with personal things for now), and ping you again if there's no performance regression. Before then, I'll add |
HeartSaVioR
changed the title
|
Personally I'd suggest to not continue this work. Duplicated code is not a big idea in performance critical code, and bringing in more function calls may make the performance more unpredictable in JVM. |
|
Ah OK. Got you. Let's spend our time for other valuable stuff. Thanks again for your opinion. Closing. |
4 participants
What changes were proposed in this pull request?
IntelliJ found lots of code duplication among some Unsafe classes, and code duplications occur from reading from/writing to unsafe object. This patch deduplicates code block around reading from/writing to unsafe object among various classes.
This would help not only reducing codes, but also let the way of dealing with unsafe object consistently.
How was this patch tested?
Existing tests.