On Mon, 1 Feb 2021 20:26:54 GMT, Andrew John Hughes <[email protected]> wrote:
> > Anybody willing to review this?
>
> I can have a go.
>
> I have two main concerns:
>
> 1. There seems to be little documentation on the new additions. I'm
> particularly concerned about things like CgroupV1Subsystem.java where a big
> chunk of documentation on the formats being read is removed and I don't see
> it being moved elsewhere. Documentation on the new getInstance methods would
> be helpful too.
The updated patch now includes some more documentation. The reason why I've
removed some of it was because the logic changed and it was outdated.
> 2. With the new volatile fields, I think it would be better if the lock
> being held to initialise them was only held when necessary to perform the
> assignment. Holding it while performing an extensive process of parsing
> multiple files that could potentially fail seems a little dangerous. i.e.
> something like:
> if (INSTANCE == null) { CGroupV1Subsystem tmp = initSubSystem(infos);
> synchronized (CgroupV1Subsystem.class) { if (INSTANCE == null) { INSTANCE =
> tmp; } } }
I've changed this as suggested, but keep in mind no parsing of multiple files
happens after this patch. It happens in CgroupSubsystemFactory.
Thanks,
Severin
-------------
PR: https://git.openjdk.java.net/jdk/pull/1393
