RFR (XS): 8030680: 292 cleanup from default method code assessment (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Wed Apr 29 12:32:37 UTC 2015

I didn't really review this but shouldn't there be a ResourceMark in the first TraceItables? and it would be better on more than one line with {}s.

http://cr.openjdk.java.net/~mhaupt/8030680/webrev.01/src/share/vm/oops/klassVtable.cpp.udiff.html

thanks, Coleen

On 4/29/15, 6:41 AM, Michael Haupt wrote:

Hi Karen,

the new webrev is at http://cr.openjdk.java.net/~mhaupt/8030680/webrev.01 - all changes applied as discussed below. Tested with: * jtreg (java.lang.invoke, java.util.stream); * vm.quick.testlist, vm.defmeth.testlist; * JPRT (HotSpot testset). Best, Michael

Am 28.04.2015 um 16:46 schrieb Karen Kinnear <karen.kinnear at oracle.com>:

Michael, I am good with all of your responses. Thanks for the updates and additional testing. many thanks, Karen On Apr 27, 2015, at 11:08 AM, Michael Haupt wrote:

Hi Karen,

thank you very much for your comments. My responses are below.

Am 23.04.2015 um 22:08 schrieb Karen Kinnear <karen.kinnear at oracle.com>: 1. In klassVtable::methodcountforinterface: A couple of optional suggestions while you are cleaning this up:

If length were initialized to 0, we would not need to have two different return locations. If the ASSERT was right before the return, i.e. not in the if (m->hasitableindex(), we would be checking the return value for all cases and it might be more future proof if someone were to extend the logic. That's a good suggestion; thanks. I've refactored the method to have one return statement only, and moved the ASSERT block to just before that. Not sure if we would need the local copy, i.e. nofmethodscopy - but I do appreciate that as also future proofing. It's more defensive now - I don't think debugging code should change local state, even if that's directly before a return. 2. initmethodMemberName I would be much more comfortable with conditional if (m.notnull() rather than assert - which only does something in debug mode. All of the checks of this sort in this method use assert() - and there are many places throughout the file (and other files) that use assert() in similar cases as well. If this should be changed to if-plus-raise-error, then that should be the case for all other similar uses of assert(), or the two that were introduced in the change are somehow special. Is this the case? I guess the assumption is that we have generated this code internally and so this should not cause a problem. 3. A favor - while you are in klassVtable.cpp ? Harold and I were reading the tracing information looking at a different bug today and realized some of the tracing is misleading. 3A: In assignitableindicesforinterface it says "Initializing itable for interface" change to "Initializing itable indices for interface" If you read the comments closely, an interface does not actually have an itable, but its methods need to have either a vtable index or an itable index. This code here assigns itable indices to methods that do not already have a vtable index (from j.l.Object) 3B: assignitableindicesforinterface if (m->hasvtableindex) ... "itable index %d for method ..." could you possibly change that to "vtable index %d for method" that would help people be less confused debugging this. Both applied. 4. Testing Thank you for the testing you already did. ... For this particular change I would ask that in addition to what you already ran you run (with a fastdebug build): - Christian Tornqvist can tell you how to run them vm.quick.testlist default methods tests (default mode and -mode invoke) jtreg java/util/streams (our favorite default method shakers) Thanks for the heads-up about testing for rt changes. I've applied the changes as described above and am now working on the testing. Will upload a revised webrev once that's successful. Best, Michael

More information about the hotspot-dev mailing list