Streamline large synch blocks in list/TMCH CA loading - #3183
Conversation
5bce4ea to
69e87a4
Compare
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on gbrodman).
core/src/main/java/google/registry/model/tld/label/ReservedList.java line 196 at r1 (raw file):
public ImmutableMap<String, ReservedListEntry> getReservedListEntries() { if (reservedListMap == null) { synchronized (reservedListMapLoadLock) {
We can use synchronized(this) and save an extra field, unless it violates some best practice?
Code quote:
synchronized (reservedListMapLoadLock)For reserved/premium lists: Use double-check locking so that subsequent calls to get the entire map of entries don't need to even check the locking object. This makes things quicker and removes lock-tracking overhead. For TMCH CA: we can just remove the synchronization block entirely. Everything inside of it is either constants (e.g. ROOT_CERTS) or a Guava loading cache (CRL_CACHE) which takes care of synchronization for us anyway.
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on weiminyu).
core/src/main/java/google/registry/model/tld/label/ReservedList.java line 196 at r1 (raw file):
Previously, weiminyu (Weimin Yu) wrote…
We can use
synchronized(this)and save an extra field, unless it violates some best practice?
eh yeah i think that only matters if we start having multiple synchronization blocks, and we're not gonna be doing any other synchronization on this object
For reserved/premium lists:
Use double-check locking so that subsequent calls to get the entire map of entries don't need to even check the locking object. This makes things quicker and removes lock-tracking overhead.
For TMCH CA:
we can just remove the synchronization block entirely. Everything inside of it is either constants (e.g. ROOT_CERTS) or a Guava loading cache (CRL_CACHE) which takes care of synchronization for us anyway.
this is taken from our self-scan D.4 14
This change is