Hi Corey, > 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?
I think the rounding logic is hard to understand and I'm not sure if it's correct (you're rounding up for the 1st computation of chars_decoded). If we don't use it, it will never get tested (because the intrinsic always returns a multiple of 3). I prefer having a more simple version which is easy to understand and for which we can test all cases. I think we should be able to catch violations of this requirement by adding good JTREG tests. An illegal intrinsic implementation should never pass the tests. So I don't see a need to catch an illegal state in the Java source code in this case. I guess this will be best for intrinsic implementors for other platforms as well. I'd appreciate more opinions on this. > I will double check that everything compiles and runs properly with gcc > 7.3.1. Please note that 7.3.1 is our minimum for Big Endian linux. For Little Endian it's 7.4.0. You can also find this information here: https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms under "Other JDK 13 build platforms" which hasn't changed since then. > I will use __attribute__ ((align(16))) instead of __vector, and make > them arrays of 16 unsigned char. Maybe __vectors works as expected, too, now. Whatever we use, I'd appreciate to double-check the alignment e.g. by using gdb. I don't remember what we had tried and why it didn't work as desired. > 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. In your code one '\' is not aligned with the other ones. > Ah, this is another thing I didn't know about. I will make some > regression tests. Thanks. There's some documentation available: https://openjdk.java.net/jtreg/ I guess your colleagues can assist you with that so you don't have to figure out everything alone. > 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. I'm glad you work on contributions. I think we should welcome new contributors and assist as far as we can. Best regards, Martin > -----Original Message----- > From: Corey Ashford <cjash...@linux.ibm.com> > Sent: Donnerstag, 27. August 2020 00:17 > To: Doerr, Martin <martin.do...@sap.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 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
