Add string-keyed fast path to NS.apply avoiding URI.create per element - #1712
Conversation
ad5839b to
8070521
Compare
jadams-tresys
left a comment
jadams-tresys
left a comment
There was a problem hiding this comment.
+1
Changes look reasonable. Have you verified any performance gains? Not sure why this didn't show up in my profiling.
Yes I did! See results below for daffodil performance unparse. I used a delimited schema with 20 plain field, 1 valuenlength OVC, interleaved with another OVC element. |
| * bounded in practice by the number of distinct namespace URI strings an | ||
| * application actually uses, which is small and fixed per schema. | ||
| */ | ||
| private val stringToNS = new java.util.concurrent.ConcurrentHashMap[String, NS]() |
There was a problem hiding this comment.
Is it possible to change our NS object to be a UniquenessCache[String,NS] so it keys off of Strings instead of URI's, and then we avoid the double caches?
We can still keep the apply(uri: URI) method for convenience, but it can just call apply(uri.toString). I believe URI.toString either returns the string it was created with, or it calculates it once and then caches that for future calls, so the extra toString shouldn't add much overhead.
| * hit there requires first parsing the string via URI.create and then taking | ||
| * a lock - fine for schema-compile-time use, but real per-call overhead on a | ||
| * per-element hot path. This map memoizes by the raw string itself, so a | ||
| * previously-seen string never reaches URI.create or the lock at all. |
There was a problem hiding this comment.
Do you know if the overhead is primarily from the locking used by the UniquenessCache or from URI.create? The benefit of the UniquenessCache over the ConcurrentHashMap is it will evict entries so we don't have an ever growing cache.
That said, I'm not sure there's really a huge concern that the cached namespaces will take up a ton of memory though, mainly because in practice there just won't be that many different namespaces used at the same time. And thinking about it more, having the ConcurrentHashMap here kind of defeats the purpose of the UniquenessCache since it's size will grow unbounded.
So I think we should either have just the UniquenessCache[String, NS] or just the ConcurrentHashMap[String, NS], and switch to the latter if we are seeing worse performance caused by the UniquenessCache locking (especially with parallel tests where the locking and multiple parallel readers/writers could become more of an issue).
There was a problem hiding this comment.
There was no discernable difference between UniquenessCache and ConcurrentHashMap implementation so I went with the UniquenessCache option. Looks like the URI.create was the overhead.
Great improvements! Is N the number of threads. Considering N=50 has a better improvement than N=20, that makes me thing the locking is a significant part of the overhead and so dropping UniquenessCache in favor of ConcurrentHashMap might be the best approach. |
No the N was number of files, I retested with different threads though and these were the results |
NextElementResolver's OnlyOnePossibilityForNextElement and SeveralPossibilitiesForNextElement both call NS(namespace) on every element resolution when the infoset source has namespaces. NS.apply(String) previously ran URI.create plus a ReentrantReadWriteLock-guarded WeakHashMap lookup on every call, even for a namespace string seen many times before. NS now extends UniquenessCache[String, NS], keyed directly by the namespace string instead of URI. A cache hit never touches URI.create; URI.create only runs in valueFromKey on a genuine cache miss. apply(URI) delegates to apply(uri.toString). DAFFODIL-3092
2c7a1ea to
6d39bd9
Compare
3 participants
NextElementResolver's OnlyOnePossibilityForNextElement and SeveralPossibilitiesForNextElement both call NS(namespace) on every element resolution when the infoset source has namespaces. NS.apply(String) previously ran URI.create plus a ReentrantReadWriteLock-guarded WeakHashMap lookup on every call, even for a namespace string seen many times before. A ConcurrentHashMap keyed by the raw string now short-circuits repeats before they ever reach URI.create or the lock.
DAFFODIL-3092