stevedlawrence commented on code in PR #1712:
URL: https://github.com/apache/daffodil/pull/1712#discussion_r3749756393
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/Namespaces.scala:
##########
@@ -42,15 +42,39 @@ object NS extends UniquenessCache[URI, NS] {
super.apply(uri)
}
+ /**
+ * Fast path for repeated raw namespace strings (e.g. resolving next-elements
+ * during unparse, where the same handful of namespace URI strings recur once
+ * per element in a document). The uniqueness cache above is a
+ * ReentrantReadWriteLock-guarded WeakHashMap keyed by URI, so even a cache
+ * 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.
+ *
+ * Unlike the uniqueness cache, entries here are never evicted; this is
+ * 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]()
Review 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.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/Namespaces.scala:
##########
@@ -42,15 +42,39 @@ object NS extends UniquenessCache[URI, NS] {
super.apply(uri)
}
+ /**
+ * Fast path for repeated raw namespace strings (e.g. resolving next-elements
+ * during unparse, where the same handful of namespace URI strings recur once
+ * per element in a document). The uniqueness cache above is a
+ * ReentrantReadWriteLock-guarded WeakHashMap keyed by URI, so even a cache
+ * 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.
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
