Quantcast

RFR(S) JDK-8178350 - klassVtable and klassItable should be ValueObj

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(S) JDK-8178350 - klassVtable and klassItable should be ValueObj

Ioi Lam
Hi, please review this JVM start-up enhancement:

https://bugs.openjdk.java.net/browse/JDK-8178350
http://cr.openjdk.java.net/~iklam/jdk10/8178350-klassvtable-as-valueobj.v01/

Klass::vtable() is called frequently inside
InstanceKlass::link_class_impl(). We have a start-up benchmark that
loads about 4000 classes. Klass::vtable() is called 32458 times and
costs about 2~3% of a total elapsed time of 720 ms.

Because klassVtable and klassItable are very simple, fixed sized
structures, it's much better to allocate them on the stack:

OLD:

class klassVtable : ResourceObj {...}
klassVtable* Klass::vtable() const {
   return new klassVtable(const_cast<Klass*>(this), start_of_vtable(),
vtable_length() / vtableEntry::size());
}

NEW:

class klassVtable VALUE_OBJ_CLASS_SPEC {...}
klassVtable Klass::vtable() const {
   return klassVtable(const_cast<Klass*>(this), start_of_vtable(),
vtable_length() / vtableEntry::size());
}

(>> David) I am not sure whether to make klassVtable a StackObj or
ValueObj. Since it's returned by a function call, it seems ValueObj is
better??

I also removed some ResourceMarks that are obviously not necessary. I
left some others unchanged, like here in link_class_impl():

  623         ResourceMark rm(THREAD);
  624         vtable().initialize_vtable(true, CHECK_false);
  625         itable().initialize_itable(true, CHECK_false);

because some code inside initialize_vtable() would do a resource
allocation without a ResourceMark. I don't want to touch that for now,
as it's unclear to me whether the allocated object(s) should be scoped,
or should be returned to a higher scope. I filed JDK-8178712 to track this.

Thanks
- Ioi
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) JDK-8178350 - klassVtable and klassItable should be ValueObj

coleen.phillimore

I like this change.  It's always been a surprise to need a ResourceMark
around klass->vtable() calls.

On 4/13/17 5:22 AM, Ioi Lam wrote:

> Hi, please review this JVM start-up enhancement:
>
> https://bugs.openjdk.java.net/browse/JDK-8178350
> http://cr.openjdk.java.net/~iklam/jdk10/8178350-klassvtable-as-valueobj.v01/ 
>
>
> Klass::vtable() is called frequently inside
> InstanceKlass::link_class_impl(). We have a start-up benchmark that
> loads about 4000 classes. Klass::vtable() is called 32458 times and
> costs about 2~3% of a total elapsed time of 720 ms.
>

Nice when cleaning something up, you get better performance!

> Because klassVtable and klassItable are very simple, fixed sized
> structures, it's much better to allocate them on the stack:
>
> OLD:
>
> class klassVtable : ResourceObj {...}
> klassVtable* Klass::vtable() const {
>   return new klassVtable(const_cast<Klass*>(this), start_of_vtable(),
> vtable_length() / vtableEntry::size());
> }
>
> NEW:
>
> class klassVtable VALUE_OBJ_CLASS_SPEC {...}
> klassVtable Klass::vtable() const {
>   return klassVtable(const_cast<Klass*>(this), start_of_vtable(),
> vtable_length() / vtableEntry::size());
> }
>
> (>> David) I am not sure whether to make klassVtable a StackObj or
> ValueObj. Since it's returned by a function call, it seems ValueObj is
> better??

I don't have a strong opinion about this.  ValueObj makes sense because
StackObj would be coded as something like, instead of this:

+ Method * object_resolved_method = object_klass->vtable().method_at(index);


Would be this:

     KlassVtable vt(object_klass);
     Method* object_resolved_method = vt.method_at(index);

So because of how it's called, it makes more sense as a ValueObj (which
we want to get rid of).  Neither can call CHEAP operator new.

>
> I also removed some ResourceMarks that are obviously not necessary. I
> left some others unchanged, like here in link_class_impl():
>
>  623         ResourceMark rm(THREAD);
>  624         vtable().initialize_vtable(true, CHECK_false);
>  625         itable().initialize_itable(true, CHECK_false);
>
> because some code inside initialize_vtable() would do a resource
> allocation without a ResourceMark. I don't want to touch that for now,
> as it's unclear to me whether the allocated object(s) should be
> scoped, or should be returned to a higher scope. I filed JDK-8178712
> to track this.
>

Seems ok to me.
thanks,
Coleen
> Thanks
> - Ioi

Loading...