Isolate LazyNodeset lock from evaluated object

[ctsims]

Product Description

Performance improvement with no user facing impact.

Technical Summary

Years ago we ran across an issue and realized that the synchronized call protecting lazy nodeset evaluations was wrapping a static global object, which meant that independent LazyNodesets would be blocked from concurrent evaluation on the same runtime.

This was fixed in #645 by making the evaluated member a unique object for each lazy nodeset instance. Unfortunately that was a shortsighted approach, as the Boolean object constructor was marked for deprecation in Java 9 and the current non-deprecated Boolean class has no matching semantics available to guarantee a unique object instance. #1202 unintentionally re-introduced the global locking semantics when the deprecated call was removed and replaced as per the javadoc guidance.

Recent profiling in Formplayer has surfaced clear instances where the requests are blocking on this lock, and CPU / Wall Clock time have significant drift where one thread is waiting for access to the synchronized boolean held by another thread's totally different nodset before being able to evaluate its own nodeset.

This time I'm separating the lock and outcome semantics entirely, which is what I should have done the first time rather than leaving a comment. It would be worth reviewing whether there are better available locks for this purpose, but this pattern (vanilla Object member) is common across our other synchronized uses so I'm not inclined to block on that for now.

Safety Assurance

Safety story

I've tested this change against the test suite, and the semantics introduced are the same as were present in the period between 2017 and 2022, which was thoroughly used.

I am not introducing new tests to confirm independent locking or replicating the failure condition in regression because the previous failure was the result of overloading intent for class member. We don't generally apply a pattern to confirm that objects with synchronized locks are working as intended.

Automated test coverage

This existing code is well covered by tests, which are passing.

QA Plan

n/a

Special deploy instructions

Formplayer will need to have its head updated after this change is merged.

Rollback instructions

• This PR can be reverted after deploy with no further considerations.

Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change.

Duplicate PR

Automatically duplicate this PR as defined in contributing.md.

[ctsims]

duplicate this PR

[dannyroberts]

There were dozens of places in #1202 that changed new X to X.valueOf. Are the rest of those safe to keep, because this is the only one calling sychronized on it?

[ctsims]

There were dozens of places in #1202 that changed new X to X.valueOf. Are the rest of those safe to keep?

Thanks for raising. I'm going to do a review of the other changes as well once this is wrapped. Fortunately in most places we were foreword looking enough to not use overloaded object references (especially for primitive wrappers) as locks, so it'd be surprising for this to have come up elsewhere.

Update: Confirmed that this was the only object ref setting a class member so there should be little concern otherwise. More relevantly, though, it's probably a good idea for us to basically review every synchronized call (we don't really use the pattern that often) and make sure there aren't other blockers.

[snopoke]

Nice find!