RFR(L): 8146401: Clean up oop.hpp: add inline directives and fix header files
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jan 5 19:24:04 UTC 2016
http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/src/share/vm/code/dependencies.cpp.udiff.html
Can you fix the preexisting weird indentation of
+ assert(_call_site() ->is_a(SystemDictionary::CallSite_klass()), "must
be");
Not sure what that was supposed to line up with originally.
http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/src/share/vm/jvmci/jvmciJavaClasses.hpp.udiff.html
Don't have an opinion about including the .inline.hpp files in these.
The compiler team should probably clean this up.
http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/src/share/vm/oops/objArrayOop.hpp.udiff.html
Why isn't inline in the declaration for obj_at_put ?
http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/src/share/vm/oops/oop.hpp.udiff.html
Why is inline commented out for some?
+ inline bool is_instance() const;
+ /*inline*/ bool is_array() const;
http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/src/share/vm/oops/oop.inline.hpp.udiff.html
I'm not really happy with the re-sorting because it may make backporting
difficult, but it's not that many that were moved and the order of these
functions has always seeemed random to me. So I guess it's ok.
Can you line up the '\' in the OOP_ITERATE macros at the end?
http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/src/share/vm/oops/typeArrayOop.cpp.udiff.html
This non-inline function could affect performance. It may be called
from GC. We might need a typeArrayOop.inline.hpp for it.
I can sponsor this, pending further review.
Coleen
On 1/4/16 10:44 AM, Lindenmaier, Goetz wrote:
> Hi,
>
> Several recent changes cleaned up includes of oop.inline.hpp in real .hpp header file.
> Unfortunately, the 'inline' qualifier is added to the function implementations
> in oop.inline.hpp instead of to the declarations in oop.hpp. Therefore, the
> compiler can not detect failing inlines properly.
>
> This change moves the inline directive from oop.inline.hpp to oop.hpp. Also
> it sorts the methods in oop.inline.hpp as they are sorted in oop.hpp.
>
> Further, it moves a row of calls from hpp files to inline.hpp or .cpp files.
>
> I put oop.inline.hpp into serviceUtil.hpp. This is not clean, but this is a
> very small .hpp file and no .cpp file exists. So I think this is acceptable.
>
> Also, I put oop.inline.hpp into jvmciJavaClasses.hpp. I don't want to do
> bigger changes to this file in the rt repo, because jvmci is subject to
> freqent changes recently.
>
> The following methods were moved to .cpp files:
>
> ProtectionDomainCacheTable::compute_hash()
> ProtectionDomainCacheTable::index_for()
> typeArrayOopDesc::object_size()
> This is called only once outside .cpp file:
> CallSiteDepChange::CallSiteDepChange()
> This is only called in .cpp file
> ConstantPool::string_at_put()
>
> If someone considers not inlining these harmful to performance,
> I will add a new .inline.hpp file for them.
>
> Please review this change. I please need a sponsor.
> There are no functional edits, so it should be simple to review.
> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/
>
> Best regards,
> Goetz.
More information about the hotspot-runtime-dev
mailing list