RFR(L): 8146395: Add inline qualifier in oop.hpp and fix inlining in gc files

Tue Jan 19 17:13:32 UTC 2016

Hi Goetz,

On 2016-01-19 17:23, Lindenmaier, Goetz wrote:
> Hi Stefan,
>
> I had to redo the old change, thus you see parts of the changes caused
> by Davids change.  And I fixed the Copyrights ... you will see :(
> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-gc/webrev.02-incr/

Ouch, yeah, that's not a pretty sight. The incremental change seems 
fine, from what I can tell.

Thanks,
StefanK

>
> Best regards,
>    Goetz.
>
>> -----Original Message-----
>> From: Stefan Karlsson [mailto:stefan.karlsson at oracle.com]
>> Sent: Dienstag, 19. Januar 2016 16:56
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-gc-
>> dev at openjdk.java.net
>> Subject: Re: RFR(L): 8146395: Add inline qualifier in oop.hpp and fix inlining in
>> gc files
>>
>> Hi Goetz,
>>
>> Would you mind sending an incremental webrev of the difference between
>> webrev.01 and webrev.02?
>>
>> Thanks,
>> StefanK
>>
>> On 2016-01-19 16:36, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> I incorporated your proposals into the change.  I also added one more
>> #include,
>>> as precompiled headers showed a build issue.
>>> Further I fixed inlines in g1OopClosures.hpp.  Because of the very same
>> issue
>>> our VS2010 build failed (see below).  I think this fits well into this change.
>>> I rebased the webrev to todays rt repository and resolved issues with
>> Davids
>>> Change.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-gc/webrev.02/
>>>
>>> This is now tested with all our tests, including hs-jtreg, linux/nt/bsd x86_64,
>>> solarissparc and our 3 ppc platforms.
>>>
>>> Best regards,
>>>     Goetz
>>>
>>> VS2010 error:
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1TriggerClosure::do_oop_work<class oopDesc *>(class oopDesc *
>> *)"
>> (??$do_oop_work at PEAVoopDesc@@@G1TriggerClosure@@QEAAXPEAPE
>> AVoopDesc@@@Z) referenced in function "public: virtual void __cdecl
>> G1TriggerClosure::do_oop(class oopDesc * *)"
>> (?do_oop at G1TriggerClosure@@UEAAXPEAPEAVoopDesc@@@Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1TriggerClosure::do_oop_work<unsigned int>(unsigned int *)"
>> (??$do_oop_work at I@G1TriggerClosure@@QEAAXPEAI at Z) referenced in
>> function "public: virtual void __cdecl G1TriggerClosure::do_oop(unsigned int
>> *)" (?do_oop at G1TriggerClosure@@UEAAXPEAI at Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1InvokeIfNotTriggeredClosure::do_oop_work<class oopDesc
>> *>(class oopDesc * *)"
>> (??$do_oop_work at PEAVoopDesc@@@G1InvokeIfNotTriggeredClosure@
>> @QEAAXPEAPEAVoopDesc@@@Z) referenced in function "public: virtual
>> void __cdecl G1InvokeIfNotTriggeredClosure::do_oop(class oopDesc * *)"
>> (?do_oop at G1InvokeIfNotTriggeredClosure@@UEAAXPEAPEAVoopDesc@
>> @@Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1InvokeIfNotTriggeredClosure::do_oop_work<unsigned
>> int>(unsigned int *)"
>> (??$do_oop_work at I@G1InvokeIfNotTriggeredClosure@@QEAAXPEAI at Z)
>> referenced in function "public: virtual void __cdecl
>> G1InvokeIfNotTriggeredClosure::do_oop(unsigned int *)"
>> (?do_oop at G1InvokeIfNotTriggeredClosure@@UEAAXPEAI at Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1Mux2Closure::do_oop_work<class oopDesc *>(class oopDesc *
>> *)"
>> (??$do_oop_work at PEAVoopDesc@@@G1Mux2Closure@@QEAAXPEAPEA
>> VoopDesc@@@Z) referenced in function "public: virtual void __cdecl
>> G1Mux2Closure::do_oop(class oopDesc * *)"
>> (?do_oop at G1Mux2Closure@@UEAAXPEAPEAVoopDesc@@@Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1Mux2Closure::do_oop_work<unsigned int>(unsigned int *)"
>> (??$do_oop_work at I@G1Mux2Closure@@QEAAXPEAI at Z) referenced in
>> function "public: virtual void __cdecl G1Mux2Closure::do_oop(unsigned int
>> *)" (?do_oop at G1Mux2Closure@@UEAAXPEAI at Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1UpdateRSOrPushRefOopClosure::do_oop_work<unsigned
>> int>(unsigned int *)"
>> (??$do_oop_work at I@G1UpdateRSOrPushRefOopClosure@@QEAAXPEAI@
>> Z) referenced in function "public: virtual void __cdecl
>> G1UpdateRSOrPushRefOopClosure::do_oop(unsigned int *)"
>> (?do_oop at G1UpdateRSOrPushRefOopClosure@@UEAAXPEAI at Z)
>>> g1RemSet.obj : error LNK2019: unresolved external symbol "public: void
>> __cdecl G1UpdateRSOrPushRefOopClosure::do_oop_work<class oopDesc
>> *>(class oopDesc * *)"
>> (??$do_oop_work at PEAVoopDesc@@@G1UpdateRSOrPushRefOopClosure
>> @@QEAAXPEAPEAVoopDesc@@@Z) referenced in function "public: virtual
>> void __cdecl G1UpdateRSOrPushRefOopClosure::do_oop(class oopDesc *
>> *)"
>> (?do_oop at G1UpdateRSOrPushRefOopClosure@@UEAAXPEAPEAVoopDesc
>> @@@Z)
>>> jvm.dll : fatal error LNK1120: 8 unresolved externals
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Stefan Karlsson [mailto:stefan.karlsson at oracle.com]
>>>> Sent: Donnerstag, 14. Januar 2016 10:12
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-gc-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR(L): 8146395: Add inline qualifier in oop.hpp and fix inlining
>> in
>>>> gc files
>>>>
>>>> Hi Goetz,
>>>>
>>>> Thanks for cleaning this up!
>>>>
>>>> You'll have some merge conflicts with:
>>>> 8146690: Make all classes in GC follow the naming convention
>>>>
>>>> So we might want to coordinate that somehow.
>>>>
>>>> Inlined:
>>>>
>>>>
>>>> On 2016-01-13 23:54, Lindenmaier, Goetz wrote:
>>>>
>>>>
>>>> 	Hi
>>>>
>>>>
>>>>
>>>> 	This change finishes the cleanup started in "8146401: Clean up
>>>> oop.hpp: add inline directives and fix header files"
>>>>
>>>> 	by fixing the issues in gc files.  See also
>>>> https://bugs.openjdk.java.net/browse/JDK-8146395
>>>> <https://bugs.openjdk.java.net/browse/JDK-8146395>  and the
>> discussion
>>>> 	of this change in the rt mailing list:
>>>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
>>>> January/017440.html> http://mail.openjdk.java.net/pipermail/hotspot-
>>>> runtime-dev/2016-January/017440.html
>>>>
>>>>
>>>>
>>>> 	Most issues can be fixed by including the proper .inline.hpp header
>>>> or moving a function there.
>>>>
>>>>
>>>>
>>>> 	I added new files
>>>> src/share/vm/gc/shared/referenceProcessor.inline.hpp and
>>>> g1SATBCardTableModRefBS.inline.hpp.
>>>>
>>>>
>>>>
>>>> 	I moved the following methods to their .cpp files. In case someone
>>>> considers these
>>>>
>>>> 	performance relevant, I will add an inline.hpp file for them:
>>>>
>>>> 	  PromotedObject::next and ::setNext    --> promotionInfo.cpp
>>>>
>>>> 	  ObjectStartArray::object_start        --> objectStartArray.cpp
>>>>
>>>> 	  FillClosure::do_addr to               --> psParallelCompact.cpp (ok, only
>>>> used here)
>>>>
>>>> 	  ageTable::add                         --> ageTable.cpp
>>>>
>>>> 	  KlassSizeStats::count_array / ::count --> heapInspection.cpp
>>>>
>>>>
>>>>
>>>> 	Please review this change.  I please need a sponsor.
>>>>
>>>> 	http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>> gc/webrev.01/ <http://cr.openjdk.java.net/%7Egoetz/wr16/8146395-
>>>> oopInline-gc/webrev.01/>
>>>>
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>>
>> gc/webrev.01/src/share/vm/gc/cms/cmsOopClosures.inline.hpp.frames.htm
>>>> l
>>>>
>>>> Why did you move trim_queues?
>>>>
>>>> I'm personally not particularly fond of these macro constructs, since the
>>>> makes it harder to navigate the code in IDEs.
>>>>     88 DO_OOP_WORK_NV_IMPL(Par_PushOrMarkClosure)
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>>
>> gc/webrev.01/src/share/vm/gc/g1/g1SATBCardTableModRefBS.hpp.udiff.ht
>>>> ml
>>>>
>>>>      // These are the more general virtual versions.
>>>> -  virtual void write_ref_field_pre_work(oop* field, oop new_val) {
>>>> -    inline_write_ref_field_pre(field, new_val);
>>>> -  }
>>>> -  virtual void write_ref_field_pre_work(narrowOop* field, oop new_val)
>> {
>>>> -    inline_write_ref_field_pre(field, new_val);
>>>> -  }
>>>> +  inline virtual void write_ref_field_pre_work(oop* field, oop new_val);
>>>> +  inline virtual void write_ref_field_pre_work(narrowOop* field, oop
>>>> new_val);
>>>>
>>>> I'm not sure how this is going to work. Have you verified that
>>>> BarrierSet::write_ref_field_pre works when called while running G1?
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>> gc/webrev.01/src/share/vm/gc/parallel/objectStartArray.cpp.udiff.html
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>> gc/webrev.01/src/share/vm/gc/parallel/objectStartArray.hpp.udiff.html
>>>>
>>>> I think object_start should be moved to an inline.hpp file, or you need to
>>>> verify that this has no performance impact.
>>>>
>>>> ---
>>>>
>>>> -#undef assert_covered_region_contains
>>>>
>>>> ^ This seems like an unrelated change.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>> gc/webrev.01/src/share/vm/gc/parallel/psScavenge.hpp.udiff.html
>>>>
>>>> There's no need for this function to be inlined.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>> gc/webrev.01/src/share/vm/gc/shared/ageTable.cpp.udiff.html
>>>>
>>>> Move to inline.hpp file.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr16/8146395-oopInline-
>>>> gc/webrev.01/src/share/vm/oops/oop.hpp.udiff.html
>>>>
>>>> Spurious new-line
>>>>
>>>> +  inline uint age() const;
>>>>      inline void incr_age();
>>>>
>>>> +
>>>>      // mark-sweep support
>>>>      void follow_body(int begin, int end);
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> 	Best regards,
>>>>
>>>> 	  Goetz Lindenmaier
>>>>
>>>>
>>>>
>>>> 	(The webrev messed up some change comments, the diffs and patch
>>>> are fine, though.)
>>>>