Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9546

There is a lot of unnecessary boxing/unboxing going on in {{SolrParams}} class

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.3, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Here is an excerpt

        public Long getLong(String param, Long def) {
          String val = get(param);
          try {
            return val== null ? def : Long.parseLong(val);
          }
          catch( Exception ex ) {
            throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, ex.getMessage(), ex );
          }
        }
      

      Long.parseLong() returns a primitive type but since method expect to return a Long, it needs to be wrapped. There are many more method like that. We might be creating a lot of unnecessary objects here.

      I am not sure if JVM catches upto it and somehow optimizes it if these methods are called enough times (or may be compiler does some modifications at compile time)

      Let me know if I am thinking of some premature optimization

      1. SOLR-9546_CloudMLTQParser.patch
        2 kB
        Pushkar Raste
      2. SOLR-9546.patch
        20 kB
        Pushkar Raste

        Issue Links

          Activity

          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

          Hmmm, that's weird... the original methods did use primitives.
          Looking at the current class, there is still the original version:

            /** Returns the long value of the param, or def if not set */
            public long getLong(String param, long def) {
              String val = get(param);
              try {
                return val == null ? def : Long.parseLong(val);
              } catch (Exception ex) {
                throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, ex.getMessage(), ex);
              }
            }
          

          But it looks like the version you reference was added more recently as part of SOLR-6485
          Anyway, I searched for usages, and that version is only used once (as part of the issue above it looks like)

          Show
          yseeley@gmail.com Yonik Seeley added a comment - Hmmm, that's weird... the original methods did use primitives. Looking at the current class, there is still the original version: /** Returns the long value of the param, or def if not set */ public long getLong( String param, long def) { String val = get(param); try { return val == null ? def : Long .parseLong(val); } catch (Exception ex) { throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, ex.getMessage(), ex); } } But it looks like the version you reference was added more recently as part of SOLR-6485 Anyway, I searched for usages, and that version is only used once (as part of the issue above it looks like)
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

          That was just one example. Check getBool()

          I was responding to "unnecessary boxing". For getBool for example, has both a boxed and primitive version:
          The boxed version is so you can tell if a value was actually present, and the primitive version can be used if you provide the primitive default.

            /** Returns the Boolean value of the param, or null if not set */
            public Boolean getBool(String param) {
              String val = get(param);
              return val==null ? null : StrUtils.parseBool(val);
            }
          
            /** Returns the boolean value of the param, or def if not set */
            public boolean getBool(String param, boolean def) {
              String val = get(param);
              return val==null ? def : StrUtils.parseBool(val);
            }
          
          Show
          yseeley@gmail.com Yonik Seeley added a comment - That was just one example. Check getBool() I was responding to "unnecessary boxing". For getBool for example, has both a boxed and primitive version: The boxed version is so you can tell if a value was actually present, and the primitive version can be used if you provide the primitive default. /** Returns the Boolean value of the param, or null if not set */ public Boolean getBool( String param) { String val = get(param); return val== null ? null : StrUtils.parseBool(val); } /** Returns the boolean value of the param, or def if not set */ public boolean getBool( String param, boolean def) { String val = get(param); return val== null ? def : StrUtils.parseBool(val); }
          Hide
          praste Pushkar Raste added a comment - - edited

          Got you.
          I will fix the Long getLong(String param, Long def) method only. It is not as bad as I initially thought.

          I don't even think that method is needed. Calling Long getLong(String param) would do the same thing, won't it?

          Show
          praste Pushkar Raste added a comment - - edited Got you. I will fix the Long getLong(String param, Long def) method only. It is not as bad as I initially thought. I don't even think that method is needed. Calling Long getLong(String param) would do the same thing, won't it?
          Hide
          praste Pushkar Raste added a comment - - edited

          I went through usage of most of the methods that return Wrapper types. I think `SolrParams` class is encouraging usage of wrapper types (or people might are just missing the fact that they might end up creating lot of wrapper objects). Here are few are some places which can use primitive types by passing a default value

          SolrParams.getInt()

          • HashQParser.parse()
          • TextLogisticRegressionQParser.parse()
          • CloudMLTQParser.parse()
          • SimpleMLTQParser.parse()

          getBool()

          • ZkController.rejoinShardElection()
          • DumpRequestHandler.handleRequestBody()
          • PingRequestHandler.handleRequestBody()
          • MoreLikeThisComponent.process()
          • BinaryResponseWriter.write()
          • JSONResponseWriter.write()
          • PHPResponseWriter.write()
          • XMLResponseWriter.write()
            JVM might do something smart for `Boolean` type, since there are only two possible values.

          There are some test classes as well.

          There are some other classes that do depend upon values being `null`.

          • I can modify all the places mentioned above to call get<Type>(param, df) version, or
          • We can simply add `getPrimitive<Type>()` methods that return default value in absence of a param, to make it clear that these methods would return a primitive

          Another possibility, I am overthinking here , and this ticket can be closed.

          Show
          praste Pushkar Raste added a comment - - edited I went through usage of most of the methods that return Wrapper types. I think `SolrParams` class is encouraging usage of wrapper types (or people might are just missing the fact that they might end up creating lot of wrapper objects). Here are few are some places which can use primitive types by passing a default value SolrParams.getInt() HashQParser.parse() TextLogisticRegressionQParser.parse() CloudMLTQParser.parse() SimpleMLTQParser.parse() getBool() ZkController.rejoinShardElection() DumpRequestHandler.handleRequestBody() PingRequestHandler.handleRequestBody() MoreLikeThisComponent.process() BinaryResponseWriter.write() JSONResponseWriter.write() PHPResponseWriter.write() XMLResponseWriter.write() JVM might do something smart for `Boolean` type, since there are only two possible values. There are some test classes as well. There are some other classes that do depend upon values being `null`. I can modify all the places mentioned above to call get<Type>(param, df) version, or We can simply add `getPrimitive<Type>()` methods that return default value in absence of a param, to make it clear that these methods would return a primitive Another possibility, I am overthinking here , and this ticket can be closed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user praste opened a pull request:

          https://github.com/apache/lucene-solr/pull/88

          SOLR-9546 Refactored to use primitives instead of wrappers

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/praste/lucene-solr SOLR-9546

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/lucene-solr/pull/88.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #88


          commit f77b31bd038abe8cb974240424fd072a35adae4f
          Author: Pushkar Raste <praste@bloomberg.net>
          Date: 2016-10-03T18:04:24Z

          SOLR-9546 Refactored to use primitives instead of wrappers


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user praste opened a pull request: https://github.com/apache/lucene-solr/pull/88 SOLR-9546 Refactored to use primitives instead of wrappers You can merge this pull request into a Git repository by running: $ git pull https://github.com/praste/lucene-solr SOLR-9546 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/88.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #88 commit f77b31bd038abe8cb974240424fd072a35adae4f Author: Pushkar Raste <praste@bloomberg.net> Date: 2016-10-03T18:04:24Z SOLR-9546 Refactored to use primitives instead of wrappers
          Hide
          praste Pushkar Raste added a comment -

          This is not a critical issue, and I might be doing premature optimization.

          Show
          praste Pushkar Raste added a comment - This is not a critical issue, and I might be doing premature optimization.
          Hide
          praste Pushkar Raste added a comment -

          This is not a critical issue, and I might be doing premature optimization.

          Show
          praste Pushkar Raste added a comment - This is not a critical issue, and I might be doing premature optimization.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit ccbafdc403fb66e4becfe1b934957f6247b07a7a in lucene-solr's branch refs/heads/master from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ccbafdc ]

          SOLR-9546: Eliminate unnecessary boxing/unboxing going on in SolrParams

          Show
          jira-bot ASF subversion and git services added a comment - Commit ccbafdc403fb66e4becfe1b934957f6247b07a7a in lucene-solr's branch refs/heads/master from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ccbafdc ] SOLR-9546 : Eliminate unnecessary boxing/unboxing going on in SolrParams
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 49ca9cea7283ab54086fdedd09889d171c777052 in lucene-solr's branch refs/heads/master from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=49ca9ce ]

          SOLR-9546: reverted some changes

          Show
          jira-bot ASF subversion and git services added a comment - Commit 49ca9cea7283ab54086fdedd09889d171c777052 in lucene-solr's branch refs/heads/master from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=49ca9ce ] SOLR-9546 : reverted some changes
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f51340993aea7cca3053844284c115bddaa90215 in lucene-solr's branch refs/heads/branch_6x from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f513409 ]

          SOLR-9546: Eliminate unnecessary boxing/unboxing going on in SolrParams

          Show
          jira-bot ASF subversion and git services added a comment - Commit f51340993aea7cca3053844284c115bddaa90215 in lucene-solr's branch refs/heads/branch_6x from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f513409 ] SOLR-9546 : Eliminate unnecessary boxing/unboxing going on in SolrParams
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7131849892f07ae9ad5cb945a138078e94fcb919 in lucene-solr's branch refs/heads/branch_6x from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7131849 ]

          SOLR-9546: reverted some changes

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7131849892f07ae9ad5cb945a138078e94fcb919 in lucene-solr's branch refs/heads/branch_6x from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7131849 ] SOLR-9546 : reverted some changes
          Hide
          praste Pushkar Raste added a comment -

          Patch for CloudMLTQParser

          Show
          praste Pushkar Raste added a comment - Patch for CloudMLTQParser
          Hide
          noble.paul Noble Paul added a comment -

          All these changes are there already in trunk?

          Show
          noble.paul Noble Paul added a comment - All these changes are there already in trunk?
          Hide
          praste Pushkar Raste added a comment -

          I think you reverted changes in the CloudMLTQParser class as some tests were failing. I added a patch SOLR-9546_CloudMLTQParser.patch only for CloudMLTQParser class

          Show
          praste Pushkar Raste added a comment - I think you reverted changes in the CloudMLTQParser class as some tests were failing. I added a patch SOLR-9546 _CloudMLTQParser.patch only for CloudMLTQParser class
          Hide
          noble.paul Noble Paul added a comment -

          Try applying the patch, it's already there

          Show
          noble.paul Noble Paul added a comment - Try applying the patch, it's already there
          Hide
          praste Pushkar Raste added a comment -

          Are you still talking about the CloundMLTQParser patch? If it was applied how come I still see code that uses objects ?

          https://github.com/apache/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/search/mlt/CloudMLTQParser.java#L72-L91

          Show
          praste Pushkar Raste added a comment - Are you still talking about the CloundMLTQParser patch? If it was applied how come I still see code that uses objects ? https://github.com/apache/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/search/mlt/CloudMLTQParser.java#L72-L91
          Hide
          noble.paul Noble Paul added a comment -

          Please try to apply your patch and let me know

          Show
          noble.paul Noble Paul added a comment - Please try to apply your patch and let me know
          Hide
          praste Pushkar Raste added a comment -

          Looks like we stepped on each other foot when I was fixing the CloudMLTQParser class. Please check updated patch.

          Show
          praste Pushkar Raste added a comment - Looks like we stepped on each other foot when I was fixing the CloudMLTQParser class. Please check updated patch.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 70b358960dfe8a6da35991b2a84c93cc9370c3d8 in lucene-solr's branch refs/heads/master from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=70b3589 ]

          SOLR-9546: remove unnecessary boxing

          Show
          jira-bot ASF subversion and git services added a comment - Commit 70b358960dfe8a6da35991b2a84c93cc9370c3d8 in lucene-solr's branch refs/heads/master from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=70b3589 ] SOLR-9546 : remove unnecessary boxing
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 25a439c51c67ab46286d6a490bd166aea793c951 in lucene-solr's branch refs/heads/branch_6x from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=25a439c ]

          SOLR-9546: remove unnecessary boxing

          Show
          jira-bot ASF subversion and git services added a comment - Commit 25a439c51c67ab46286d6a490bd166aea793c951 in lucene-solr's branch refs/heads/branch_6x from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=25a439c ] SOLR-9546 : remove unnecessary boxing
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 70b358960dfe8a6da35991b2a84c93cc9370c3d8 in lucene-solr's branch refs/heads/feature/metrics from Noble Paul
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=70b3589 ]

          SOLR-9546: remove unnecessary boxing

          Show
          jira-bot ASF subversion and git services added a comment - Commit 70b358960dfe8a6da35991b2a84c93cc9370c3d8 in lucene-solr's branch refs/heads/feature/metrics from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=70b3589 ] SOLR-9546 : remove unnecessary boxing
          Hide
          janhoy Jan Høydahl added a comment -

          This jira appears in CHANGES.txt for 6.3.0, please tag appropriately and close.

          Show
          janhoy Jan Høydahl added a comment - This jira appears in CHANGES.txt for 6.3.0, please tag appropriately and close.
          Hide
          varunthacker Varun Thacker added a comment -

          Marking this issue as resolved.

          Show
          varunthacker Varun Thacker added a comment - Marking this issue as resolved.

            People

            • Assignee:
              noble.paul Noble Paul
              Reporter:
              praste Pushkar Raste
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development