8195148: Collapse G1SATBCardTableModRefBS and G1SATBCardTableLoggingModRefBS into a single G1BarrierSet
erik.osterlund at oracle.com
Tue Mar 6 07:07:48 UTC 2018
On 6 Mar 2018, at 04:56, Kim Barrett <kim.barrett at oracle.com> wrote:
>> On Mar 5, 2018, at 5:11 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>> Hi Kim,
>> On 2018-03-05 23:02, Kim Barrett wrote:
>>>> On Mar 5, 2018, at 4:10 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>>> It breaks an include cycle that is necessary for ZGC (and probably Shenandoah too) to build with #include "oops/oop.inline.hpp" being in g1BarrierSet.inline.hpp. It reproduces when files include both barrierSet.inline.hpp and access.inline.hpp. Because barrierSet.inline.hpp includes barrierSetConfig.inline.hpp which includes all concrete barrier sets, which now also includes oop.inline.hpp which includes access.inline.hpp which includes barrierSet.inline.hpp again. This cycle causes resulution to barrierset accessors to happen before their metafunctions have been declared that translate barrierset types to/from enum values, which breaks the build. This bad cycle is broken with these changes by having access.inline.hpp include barrierSetConfig.inline.hpp instead of barrierSet.inline.hpp so that the barrierset type translation metafunctions have always been declared when resulution is defined.
>>> I suspected it was something like that. Can you provide more detail,
>>> or much better, tell me how to reproduce the problem? In the absence
>>> of such, the proposed changes look kind of ad hoc and fragile to me.
>>> I've been thinking about some questions about how we write and use
>>> .inline.hpp files, and have some ideas that may or may not be
>>> relevant. If I could reproduce the problem at hand, I might be able
>>> to suggest some alternative ideas. Depending on what I find, I might
>>> say go ahead with the proposed change, but I'd like to have a look
>>> first, in case there's a (relatively) simple alternative that seems
>>> more solid.
>> The easiest way to reproduce is to either:
>> 1) Add #include "oops/oop.inline.hpp" in g1SATBCardTableModRefBS.inline.hpp in the Z repo, or:
>> 2) Add any minimal no-op barrier set to jdk-hs with a name that comes after "g1" in lexicographical order (and hence include order), and plug it in to the barrierSetConfig files, and then add #include "oops/oop.inline.hpp" to the g1SATBCardTableModRefBS.inline.hpp file.
> Thanks for the instructions for reproducing. I was able to create a minimal no-op barrier set and
> did indeed get compile errors.
> Now that I understand the problem, your proposed include changes look like a good minimal fix.
> I think there are deeper issues regarding .inline.hpp files, but that can be discussed separately and
> without delaying this change any further.
I am looking forward to what comes out from that discussion. Would be great with a more solid solution for managing dependencies between .inline.hpp files.
> Looks good.
Thank you for the review.
More information about the hotspot-dev