activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jbertram <...@git.apache.org>
Subject [GitHub] activemq-artemis pull request #2228: ARTEMIS-2017 SelectorParser cache not t...
Date Wed, 08 Aug 2018 17:27:42 GMT
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2228#discussion_r208667762
  
    --- Diff: artemis-selector/src/main/java/org/apache/activemq/artemis/selector/impl/SelectorParser.java
---
    @@ -80,11 +78,15 @@ public static BooleanExpression parse(String sql) throws FilterException
{
                    StrictParser parser = new StrictParser(new StringReader(actual));
                    e = parser.JmsSelector();
                 }
    -            cache.put(sql, e);
    +            synchronized (cache) {
    --- End diff --
    
    I imagine there might be a "better" option here in terms of performance than using `synchronized`,
but then again this isn't exactly on the hot path.  This code is only executed when a selector/filter
is first created (e.g. for a JMS consumer, for a bridge, etc.), and it is only executed once
(i.e. execution isn't looped).  Most situations where cache growth is problematic (e.g. lots
of consumers with lots of different selectors connecting and disconnecting often) are generally
considered messaging anti-patterns anyway.  For such pathological cases using `synchronized`
is potentially quite a bit faster than a complete lack of thread safety because it prevents
unfettered cache growth.  In my opinion @franz1981's time is better spent elsewhere as this
smacks of premature optimization.


---

Mime
View raw message