6 ms·

Man, I always use String.intern() for synchronized(theString). -- https://shipilev.net/jvm/anatomy-quarks/10-string-intern/ https://shipilev.net/jvm/anatomy-qua

Man, I always use String.intern() for synchronized(theString). -- https://shipilev.net/jvm/anatomy-quarks/10-string-intern/ https://shipilev.net/jvm/anatomy-quarks/10-string-intern/

aardvark179 7y ago
I hesitate to ask what exactly you’re trying to achieve locking On a string which you have to intern like that. There may well be a different design that would avoid having to do it.
- deleted 7y ago
  [deleted]
diroussel 7y ago
If need to synchronise() and there is not an obvious choice then I create a new Object() for that purpose and make sure it’s available wherever locking is needed (ideally encapsulated in one class)
- sorokod 7y ago
  The ReentrantLock class and friends have been around for a while and typically provide a better alternative.
  - xxs 7y ago
    Nowadays (java 7/8), "synchronize" is likely better than most uses of reentrant lock, though. It's especially good in (common) cases where the lock is not contended. even CHM uses synchronized nowadays. Flip note: don't touch ReadWriteLocks
    - sorokod 7y ago
      > don't touch ReadWriteLocks I am interested in references that back this statement
      - jabba_d_hut 7y ago
        StampedLocks performs much better than R/W locks.
      - MrBuddyCasino 7y ago
        I was interested in this as well, and yes ReadWriteLocks are horrible[0]. Just using synchronized is a good default that performs well in most scenarios, StampedLocks are good too. [0] https://blog.overops.com/java-8-stampedlocks-vs-readwritelocks-and-synchronized/ https://blog.overops.com/java-8-stampedlocks-vs-readwriteloc...
        xxs 7y ago
        About the blog - testing with more threads than cores could be quite misleading, also testing on dual/quad socket vs single one exhibits the effects on coherency traffic a lot more (compared to L3 talk)
      - xxs 7y ago
        Briefly: - the lock has write a CAS on the =fast= read path, causing coherency traffic and a contention point between the readers. That's it the readers don't scale - it's quite hard to use correctly, i.e. after read, determining the exclusive/write lock has to be acquired, the read lock has to be released 1st, the write lock acquired and the conditions that causes the grab to be rechecked - Copy-On-Write should be a preferred solution for most cases, easy to understand and reason about. If not StampedLock is a better alternative.
    - diroussel 7y ago
      In the past I have followed this approach myself. But that is changing. If you want to future proof your code, don't use classic locks. Project Loom doesn't work with classic locks, and the java.nio package is has been re-written to remove classic locks: > In Project Loom, there will be support for efficiently switching between fibers that use Java 5 locks (that is, the java.util.concurrent.lock package) but not native C locks. As a result, it is necessary to migrate all blocking code in the JDK over to Java 5 locks. So the legacy Socket API required reimplementation to achieve better compatibility with Project Loom. See: https://blogs.oracle.com/javamagazine/inside-java-13s-switch-expressions-and-reimplemented-socket-api https://blogs.oracle.com/javamagazine/inside-java-13s-switch...
vbezhenar 7y ago
Avoiding this pattern is surprisingly hard to get right.
hyperpape 7y ago
Aside from String.intern() being dubious, if you synchronized on an interned string, anything else that happens to work with an interned copy of that string is now contending with your lock. In the worst case, you could deadlock. If you really need this sort of dynamic locking, you can use a ConcurrentHashMap<String, Object> to achieve it (lock on the object). I'm not sure whether it's ever the _best_ design, but it avoids interning the string, and it keeps an anonymous lock object that you know won't be shared.
- sk5t 7y ago
  Yeah--mucking about with .intern for no good reason, or locking on strings in general, shouldn't pass a code review, this reeks of sticking one's fingers in all sorts of places they don't belong. ConcurrentHashMap.computeIfAbsent (and computing a value that is _not_ subject to mysterious external forces) is so much better.
  - xxs 7y ago
    >ConcurrentHashMap.computeIfAbsent It's leak prone as it'd hold the references forever. It's a cheap and easy way to do it but far from ideal. I'd not recommend it.
    - sk5t 7y ago
      This seems a strange criticism--of course entries in a map don't disappear for no reason at all. Consider using an atomic cache if you are going to stick so many keys into the map that it becomes an issue. Or else bucket the keys into a known set, if you can tolerate the occasional collision.
      - xxs 7y ago
        String.intern does remove references (it used not to and it was considered a major issue). Imagine parsing documents and keeping the words - it works well as long as the input is not malicious. Otherwise words would pile up with no one cleaning them, give it enough time/input to slow the application due to memory pressure, crashing with OOM.
xxs 7y ago
Sync on a string is a bad to boot. It's hard to explain how terrible the idea to synchronize/lock objects you don't control. As for interning (not for strings only and not guaranteed) I have a lock free table (not CHM) that keeps most used objects (with possible random eviction) to provide a good trade off between unnecessary memory waste, fast access and low memory footprint.

Man, I always use String.intern() for synchronized(theString). -- https://shipilev.net/jvm/anatomy-quarks/10-string-intern/ https://shipilev.net/jvm/anatomy-quarks/10-string-intern/

aardvark179 7y ago

I hesitate to ask what exactly you’re trying to achieve locking On a string which you have to intern like that. There may well be a different design that would avoid having to do it.

deleted 7y ago
[deleted]

diroussel 7y ago

If need to synchronise() and there is not an obvious choice then I create a new Object() for that purpose and make sure it’s available wherever locking is needed (ideally encapsulated in one class)

sorokod 7y ago
The ReentrantLock class and friends have been around for a while and typically provide a better alternative.
- xxs 7y ago
  Nowadays (java 7/8), "synchronize" is likely better than most uses of reentrant lock, though. It's especially good in (common) cases where the lock is not contended. even CHM uses synchronized nowadays. Flip note: don't touch ReadWriteLocks
  - sorokod 7y ago
    > don't touch ReadWriteLocks I am interested in references that back this statement
    - jabba_d_hut 7y ago
      StampedLocks performs much better than R/W locks.
    - MrBuddyCasino 7y ago
      I was interested in this as well, and yes ReadWriteLocks are horrible[0]. Just using synchronized is a good default that performs well in most scenarios, StampedLocks are good too. [0] https://blog.overops.com/java-8-stampedlocks-vs-readwritelocks-and-synchronized/ https://blog.overops.com/java-8-stampedlocks-vs-readwriteloc...
      - xxs 7y ago
        About the blog - testing with more threads than cores could be quite misleading, also testing on dual/quad socket vs single one exhibits the effects on coherency traffic a lot more (compared to L3 talk)
    - xxs 7y ago
      Briefly: - the lock has write a CAS on the =fast= read path, causing coherency traffic and a contention point between the readers. That's it the readers don't scale - it's quite hard to use correctly, i.e. after read, determining the exclusive/write lock has to be acquired, the read lock has to be released 1st, the write lock acquired and the conditions that causes the grab to be rechecked - Copy-On-Write should be a preferred solution for most cases, easy to understand and reason about. If not StampedLock is a better alternative.
  - diroussel 7y ago
    In the past I have followed this approach myself. But that is changing. If you want to future proof your code, don't use classic locks. Project Loom doesn't work with classic locks, and the java.nio package is has been re-written to remove classic locks: > In Project Loom, there will be support for efficiently switching between fibers that use Java 5 locks (that is, the java.util.concurrent.lock package) but not native C locks. As a result, it is necessary to migrate all blocking code in the JDK over to Java 5 locks. So the legacy Socket API required reimplementation to achieve better compatibility with Project Loom. See: https://blogs.oracle.com/javamagazine/inside-java-13s-switch-expressions-and-reimplemented-socket-api https://blogs.oracle.com/javamagazine/inside-java-13s-switch...

vbezhenar 7y ago

Avoiding this pattern is surprisingly hard to get right.

hyperpape 7y ago

Aside from String.intern() being dubious, if you synchronized on an interned string, anything else that happens to work with an interned copy of that string is now contending with your lock. In the worst case, you could deadlock. If you really need this sort of dynamic locking, you can use a ConcurrentHashMap<String, Object> to achieve it (lock on the object). I'm not sure whether it's ever the _best_ design, but it avoids interning the string, and it keeps an anonymous lock object that you know won't be shared.

sk5t 7y ago
Yeah--mucking about with .intern for no good reason, or locking on strings in general, shouldn't pass a code review, this reeks of sticking one's fingers in all sorts of places they don't belong. ConcurrentHashMap.computeIfAbsent (and computing a value that is _not_ subject to mysterious external forces) is so much better.
- xxs 7y ago
  >ConcurrentHashMap.computeIfAbsent It's leak prone as it'd hold the references forever. It's a cheap and easy way to do it but far from ideal. I'd not recommend it.
  - sk5t 7y ago
    This seems a strange criticism--of course entries in a map don't disappear for no reason at all. Consider using an atomic cache if you are going to stick so many keys into the map that it becomes an issue. Or else bucket the keys into a known set, if you can tolerate the occasional collision.
    - xxs 7y ago
      String.intern does remove references (it used not to and it was considered a major issue). Imagine parsing documents and keeping the words - it works well as long as the input is not malicious. Otherwise words would pile up with no one cleaning them, give it enough time/input to slow the application due to memory pressure, crashing with OOM.

xxs 7y ago

Sync on a string is a bad to boot. It's hard to explain how terrible the idea to synchronize/lock objects you don't control. As for interning (not for strings only and not guaranteed) I have a lock free table (not CHM) that keeps most used objects (with possible random eviction) to provide a good trade off between unnecessary memory waste, fast access and low memory footprint.