RFR (S): 8022494: Make compilation IDs sequential (original) (raw)

Christian Thalinger christian.thalinger at oracle.com
Thu Jan 9 10:56:35 PST 2014

Looks good. Although I would’ve put the comment here:

On Jan 9, 2014, at 6:50 AM, Albert Noll <albert.noll at oracle.com> wrote:

Hi Chris,

thanks for looking at this again. I added a comment and fixed the typo. Here is the new webrev: http://cr.openjdk.java.net/~anoll/8022494/webrev.07/ Best, Albert

On 01/08/2014 09:34 PM, Christian Thalinger wrote: One thing we might want to add a comment for is that in a product build we only increase compilationid and not osrcompilationid. This is fine because CICountOSR is a develop flag with a default value of false but it is confusing to the reader.

+ id = Atomic::add(1, &osr2compilationid); Typo. Otherwise this looks good. On Jan 6, 2014, at 11:23 PM, Albert Noll <albert.noll at oracle.com> wrote:

Hi Vladimir,

sorry, I misunderstood your suggestion, Now it makes sense. Here is the new webrev that contains your proposed solution. http://cr.openjdk.java.net/~anoll/8022494/webrev.06/ Best, Albert On 01/06/2014 11:36 PM, Vladimir Kozlov wrote: Albert,

Next comment does not sound correct: ! // These counters are used to assign each compilation an unique ID I think the original was more correct with small correction: ! // These counters are used to assign an unique ID to each compilation

And you did not fix it as I asked: I suggested to generate compileid always in such case and convert your warning to assert (since it could only happens in debug VM). I suggested next: int CompileBroker::assigncompileid(methodHandle method, int osrbci) { #ifdef ASSERT bool isosr = (osrbci != standardentrybci); int id; if (method->isnative()) { assert(!isosr, "can't be osr"); // Adapters, native wrappers and method handle intrinsics // should be generated always. return Atomic::add(1, &compilationid); } else if (CICountOSR && isosr) { id = Atomic::add(1, &osrcompilationid); if (CIStartOSR <= id && id < CIStopOSR) {_ _return id;_ _}_ _} else {_ _id = Atomic::add(1, &compilationid);_ _if (CIStart <= id && id < CIStop) {_ _return id;_ _}_ _}_ _// Method was not in the appropriate compilation range._ _method->setnotcompilablequietly(); return 0; #else return Atomic::add(1, &compilationid); #endif } The assert should stay in createnativewrapper() as in your previous version: + const int compileid = CompileBroker::assigncompileid(method, CompileBroker::standardentrybci); + assert(compileid > 0, "Must generate native wrapper"); Thanks, Vladimir On 1/6/14 12:07 AM, Albert Noll wrote: Hi Vladimir, thanks for your explanation. I agree with your suggestion. The new version has the corresponding check inside CompileBroker::assigncompileid() Also why you return only in such case and not for normal native wrappers? Thanks for catching this bug. I fixed it in the new version.

Here is the link to the new webrev: http://cr.openjdk.java.net/~anoll/8022494/webrev.05/ Best, Albert On 12/21/2013 08:46 PM, Vladimir Kozlov wrote: We have to generate methodhandleintrinsic so we can't simple show warning and continue execution - we can't do that. I suggested to generate compileid always in such case and convert your warning to assert (since it could only happens in debug VM). Also why you return only in such case and not for normal native wrappers? + if (method->ismethodhandleintrinsic()) { + warning("Must generate wrapper for method handle intrinsic"); + return; + } --- + assert(!method->ismethodhandleintrinsic()), "Must generate wrapper for method handle intrinsic"); + return; Thanks, Vladimir On 12/18/13 10:18 PM, Albert Noll wrote: Christian, Vladimir, thanks for the review. @Christian: Thanks for catching the typo @Vladimir: I am not sure if I understand your suggestion correctly. Could you please clarify what you mean by "The warning above will be assert after that." Best, Albert

On 10/28/2013 07:59 PM, Vladimir Kozlov wrote: Albert, The warning is not correct solution since we HAVE to generate method handle intrinsics if your comment is correct: + // must be generated for method handle intrinsics (8026407), print out a warning. + if (method->ismethodhandleintrinsic()) { + warning("Must generate wrapper for method handle intrinsic"); + return; + } I think assigncompileid() should generate id in such case regardless CIStart and CIStop values. The warning above will be assert after that. And, please, file RFE (starter task) to cleanup type of compileid. In some places it declared as 'int' and in an other as 'uint'. Thanks, Vladimir On 10/24/13 1:56 AM, Albert Noll wrote: Here is the updated webrev: http://cr.openjdk.java.net/~anoll/8022494/webrev.04/ <http://cr.openjdk.java.net/%7Eanoll/8022494/webrev.04/> Best, Albert On 24.10.2013 10:21, Albert Noll wrote: Hi Aleksey, thanks for looking at this. On 24.10.2013 10:15, Aleksey Shipilev wrote: On 10/24/2013 12:01 PM, Albert Noll wrote: Here is the updated webrev: http://cr.openjdk.java.net/~anoll/8022494/webrev.03/ Nice to see the locking gone.

compileBroker.cpp: * Is that considered correct that OSR and normal compilations are marked differently when running in debug mode, but not in release? I understand the comment before assigncompileid, so this is more of the philosophical question. Compilation IDs are only different if -XX:CICountOSR is set, which is defaulted to false. sharedRuntime.cpp: * Why do you need "2653 return;" in the method tail? Thanks for spotting this. I missed it during the cleanup. Best, Albert Thanks, -Aleksey.

More information about the hotspot-compiler-dev mailing list