Hi Martin,
Some inline responses below.
On 8/26/20 8:26 AM, Doerr, Martin wrote:
Hi Corey,
I should explain my comments regarding Base64.java better.
Let's be precise: "should process a multiple of four" => "must process a
multiple of four"
Did you try to support non-multiple of 4 and this was intended as
recommendation?
I think making it a requirement and simplifying the logic in decode0 is better.
Or what's the benefit of the recommendation?
If I make a requirement, I feel decode0 should check that the
requirement is met, and raise some kind of internal error if it isn't.
That actually was my first implementation, but I received some comments
during an internal review suggesting that I just "round down" the
destination count to the closest multiple of 3 less than or equal to the
returned value, rather than throw an internal exception which would
confuse users. This "enforces" the rule, in some sense, without error
handling. Do you have some thoughts about this?
If any illegal base64 bytes are encountered in the source by the
intrinsic, the intrinsic can return a data length of zero or any
number of bytes before the place where the illegal base64 byte
was encountered.
I think this has a drawback. Somebody may use a debugger and want to stop
when throwing IllegalArgumentException. He should see the position which
matches the Java implementation.kkkk
This is probably hard to understand. Let me try to explain it by example:
1. 80 Bytes get processed by the intrinsic and 60 Bytes written to the
destination array.
2. The intrinsic sees an illegal base64 Byte and it returns 12 which is allowed
by your specification.
3. The compiled method containing the intrinsic hits a safepoint (e.g. in the
large while loop in decodeBlockSlow).
4. A JVMTI agent (debugger) reads dp and dst.
5. The person using the debugger gets angry because more bytes than dp were
written into dst. The JVM didn't follow the specified behavior.
I guess we can and should avoid it by specifying that the intrinsic needs to
return the dp value matching the number of Bytes written.
That's an interesting point. I will change the specification, and the
intrinsic implementation. Right now the Power9/10 intrinsic returns 0
when any illegal character is discovered, but I've been thinking about
returning the number of bytes already written, which will allow
decodeBlockSlow to more quickly find the offending character. This
provides another good reason to make that change.
Best regards,
Martin
-----Original Message-----
From: Doerr, Martin
Sent: Dienstag, 25. August 2020 15:38
To: Corey Ashford <cjash...@linux.ibm.com>; Michihiro Horie
<ho...@jp.ibm.com>
Cc: hotspot-compiler-...@openjdk.java.net; core-libs-dev <core-libs-
d...@openjdk.java.net>; Kazunori Ogata <oga...@jp.ibm.com>;
jos...@br.ibm.com
Subject: RE: RFR(M): 8248188: [PATCH] Add HotSpotIntrinsicCandidate and
API for Base64 decoding
Hi Corey,
thanks for proposing this change. I have comments and suggestions
regarding various files.
Base64.java
This is the only file which needs another review from core-libs-dev.
First of all, I like the idea to use a HotSpotIntrinsicCandidate which can
consume as many bytes as the implementation wants.
Comment before decodeBlock:
Let's be precise: "should process a multiple of four" => "must process a
multiple of four"
If any illegal base64 bytes are encountered in the source by the
intrinsic, the intrinsic can return a data length of zero or any
number of bytes before the place where the illegal base64 byte
was encountered.
I think this has a drawback. Somebody may use a debugger and want to stop
when throwing IllegalArgumentException. He should see the position which
matches the Java implementation.
Please note that the comment indentation differs from other comments.
Will fix.
decode0: Final "else" after return is redundant.
Will fix.
stubGenerator_ppc.cpp
"__vector" breaks AIX build!
Does it work on Big Endian linux with old gcc (we require 7.3.1, now)?
Please either support Big Endian properly or #ifdef it out.
I have been compiling with only Advance Toolchain 13, which is 9.3.1,
and only on Linux. It will not work with big endian, so it won't work
on AIX, however obviously it shouldn't break the AIX build, so I will
address that. There's code to set UseBASE64Intrinsics to false on big
endian, but you're right -- I should ifdef all of the intrinsic code for
little endian for now. Getting it to work on big endian / AIX shouldn't
be difficult, but it's not in my scope of work at the moment.
I will double check that everything compiles and runs properly with gcc
7.3.1.
What exactly does it (do) on linux?
It's an arch-specific type that's 16 bytes in size and aligned on a
16-byte boundary.
I remember that we had tried such prefixes but were not satisfied. I think it
didn't enforce 16 Byte alignment if I remember correctly.
I will use __attribute__ ((align(16))) instead of __vector, and make
them arrays of 16 unsigned char.
Attention: C2 does no longer convert int/bool to 64 bit values (since JDK-
8086069). So the argument registers for offset, length and isURL may contain
garbage in the higher bits.
Wow, that's good to know! I will mask off the incoming values.
You may want to use load_const_optimized which produces shorter code.
Will fix.
You may want to use __ align(32) to align unrolled_loop_start.
Will fix.
I'll review the algorithm in detail when I find more time.
assembler_ppc.hpp
assembler_ppc.inline.hpp
vm_version_ppc.cpp
vm_version_ppc.hpp
Please rebase. Parts of the change were pushed as part of 8248190: Enable
Power10 system and implement new byte-reverse instructions
Will do.
vmSymbols.hpp
Indentation looks odd at the end.
I was following what was done for encodeBlock, but it appears
encodeBlock's style isn't what is used for the other intrinsics. I will
correct decodeBlock to use the prevailing style. Another patch should
be added (not part of this webrev) to correct encodeBlock's style.
library_call.cpp
Good. Indentation style of the call parameters differs from encodeBlock.
Will fix.
runtime.cpp
Good.
aotCodeHeap.cpp
vmSymbols.cpp
shenandoahSupport.cpp
vmStructs_jvmci.cpp
shenandoahSupport.cpp
escape.cpp
runtime.hpp
stubRoutines.cpp
stubRoutines.hpp
vmStructs.cpp
Good and trivial.
Tests:
I think we should have JTREG tests to check for regressions in the future.
Ah, this is another thing I didn't know about. I will make some
regression tests.
Thanks for your time on this. As you can tell, I'm inexperienced in
writing openjdk code, so your patience and careful review is really
appreciated.
- Corey
