RFR: 8347337: ZGC: String dedups short-lived strings [v2]

[Stefan Karlsson]

On Thu, 13 Mar 2025 21:32:09 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> This was intentional. I named them zStringDedup.* to show that this is the file that contains ZGC's support to interface with StringDedup. We do that for other sub-systems that ZGC interfaces with.
>> 
>> The crux is that the string dedup API requires us to maintain the lifecycle of a `StringDedup::Requests` instance, so we can't simply have a function like `ZStringDedup::request(obj)`. Instead we need to add a ZStringDedupContext class, just so that we maintain the Requests object. (I choose suffix `Context` instead of `Requests`, because that naming fits better with the rest of the ZGC code).
>> 
>> About the style _guide_. I see that section more as a helpful guide, but not as a complete mandate to how to name files in HotSpot.
>> 
>> Note, that even the StringDedup::Request class is placed in a file named stringDedup.hpp and not stringDedupRequest.hpp! I could easily also create a structure that simulates the structure used in stringDedup.hpp:
>> 
>> 
>> class ZStringDedup {
>> public:
>>   class Context {
>>     ...
>>   };
>> };
>> 
>> 
>> However, I don't find that particularly appealing and it somewhat goes against the informal style guide that Per and I used when we first started to write ZGC.
>
>> About the style _guide_. I see that section more as a helpful guide, but not
>> as a complete mandate to how to name files in HotSpot.
> 
> That's true for a lot of the style guide.  But there generally ought to be a
> (good) reason to be different.  I don't see a particularly compelling reason
> in this case, though zgc has a number of stylistic peculiarities of its own,
> and maybe that's sufficient.
> 
>> Note, that even the StringDedup::Request class is placed in a file named
>> stringDedup.hpp and not stringDedupRequest.hpp!
> 
> The StringDedup class (and associated header file) is the external interface
> to the facility, so having the Request class be a member is natural. As such
> it's natural to treat it as a "buddy" class in the sense used in the style
> guide. So I don't see that as a compelling argument for the proposed zgc
> structure.
> 
>> I could easily also create a structure that simulates the structure used in
>> stringDedup.hpp:
> 
> Given that the Context class would be the only thing in such a notional
> ZStringDedup class, I agree that doesn't seem like an improvement.
> 
>> This was intentional. I named them zStringDedup.* to show that this is the
>> file that contains ZGC's support to interface with StringDedup. We do that for
>> other sub-systems that ZGC interfaces with.
> 
> If I was looking for StringDedup-related stuff in zgc and only found files
> named `zStringDedupContext.*`, I would certainly look there. Though I admit that
> if I was looking for the class ZStringDedupContext I would look in
> `zStringDedup.*` in the absence of any other `zStringDedup*.*`.
> 
> I don't feel super strongly about this, and it seems you do, so I'm not going
> to block over it.

Right, where you state that you don't see a compelling reason I do see them, so thanks for not blocking this.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23965#discussion_r1998085535