reusing empty NamedList rather than recreating a new empty NamedList … #2932

renatoh · 2025-01-03T09:06:12Z

For every call with face=true which is not providing any facet.fields, we do create an empty NamedList.
Having a single instance of an empty NamedList feels like the more efficient approach to me.

…every time the facet.field parameter is empty

renatoh · 2025-01-03T17:36:26Z

I've investigated why the tests are now failing.
The issue is that the method org.apache.solr.request.SimpleFacets#getFacetFieldCounts is now returning an empty NamedList, and not its subtype SimpleOrderedMap anymore. This seems fine since the return type of that method is NamedList.
The problem is that returning NamedList causes that in the response-json, the facet_counts/facet_fields will become a list:

The method org.apache.solr.common.util.JsonTextWriter#writeNamedList distinguish between these types and writes a different data structure.

I think the root cause is the method org.apache.solr.request.SimpleFacets#getFacetFieldCounts allowing to return the super-type NamedList.
Shall I change the return type of that method to SimpleOrderedMap?

madrob · 2025-01-03T19:56:03Z

@dsmiley this looks related to your named list vs simple map thread on dev list

renatoh · 2025-01-04T08:32:38Z

@madrob do you have a link to the mention thread?

madrob · 2025-01-04T19:46:29Z

https://lists.apache.org/thread/gllkpo16wotmkq0ppsq578yn7dhkyo9q

dsmiley · 2025-01-05T06:57:44Z

My general opinion here is wondering it it worth bothering with the micro optimization of using a shared instance. And more importantly, there is no immutable NamedList so I'm uncomfortable with introducing a shared mutable instance.

renatoh · 2025-01-05T11:01:51Z

@dsmiley I do get your point that a shared instance should be immutable. I could instantiate the shared empty instances with an immutable list, similar to what we do here:
org.apache.solr.common.util.NamedList#getImmutableCopy.

For my understanding: What is the purpose of SimpleOrderedMap, it is just to control the output format of the response writers?

dsmiley · 2025-01-05T15:47:43Z

Yes, it's about the response format but moreover it communicates that keys do not (well technically "should" not) repeat. It's tempting to actually implement Map on this thing.

renatoh · 2025-01-05T17:36:50Z

Not being very familiar with the Solr source code, this construct with NamedList and SimpleOrderedMap is pretty confusing to me.
From what I understand NamedList is basically a List of Pair<Key,Value> (Ordered and keys can repeat) and SimpleOrderedMap is an HashMap or LinkedHashMap (not necessarily ordered but with unique) . Is that understanding correct?

Back in 2006 NamedList had the following todo on it:

:TODO: In the future, it would be nice if this extended Map or Collection,
had iterators, used java5 generics, had a faster lookup for
large lists, etc...
It could also have an interface, and multiple implementations.
One might have indexed lookup, one might not.

But this one got removed at one point

dsmiley · 2025-01-06T04:29:17Z

NamedList and thus SimpleOrderedMap is ordered (insertion order). HashMap is called HashMap because it explains its implementation via use of the hashCode() and thus get(key) is O(1). NamedList and thus SimpleOrderedMap doesn't use a hashCode so I would say it's not specifically like a HashMap but I would indeed say that SimpleOrderedMap is a Map even though it doesn't implement the interface. It has Map in its name, after all. I recall there was concern that implementing Map would lead to people passing this off to code that makes assumptions on Map having better than O(N) lookup by key (even TreeMap is O(Log N)) but NamedList is O(N) leading to a performance problem for non-trivial sized NamedLists.

renatoh · 2025-01-06T15:50:10Z

Thanks for the explanation.
Would another option be to use a Map internally for SimpleOrderedMap without changing the interface, that lead to better lookup performance without the need of touching a lot of code?

dsmiley · 2025-01-06T17:22:40Z

Adding an additional HashMap inside SimpleOrderedMap misses the point of why NamedList exists. It's for something super lean/fast that's only for transferring stuff (i.e. to be serialized), navigated by iteration once. For that use-case, a hashCode system is a waste. But Solr uses it for configuration as well, a questionable choice that's not in line with NamedList's advantages. Perhaps a radical point of view is that NamedList shouldn't even have "get" methods, leaving only iteration, thus emphasizing its purpose. For sure, Solr over-uses NamedList. And it's hard to stop using it because of SolrJ "javabin" compatibility.

dsmiley · 2025-01-08T21:45:37Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

@@ -34,6 +35,8 @@
 * serialized. It aims to minimize overhead and to be efficient at adding new elements.
 */
 public class SimpleOrderedMap<T> extends NamedList<T> {
+
+  private static final SimpleOrderedMap<Object> emptySimpleOrderedMap = new SimpleOrderedMap<>(Collections.emptyList());


call it EMPTY ?

Also nowadays we just do List.of() which is equivalent, shorter, and doesn't import another class.

" and doesn't import another class." Never thought of that advantages of List.of over Collections.empty(). Good point, will adjust it!

dsmiley · 2025-01-08T21:51:37Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

@@ -67,4 +70,8 @@ public SimpleOrderedMap<T> clone() {
    newList.addAll(nvPairs);
    return new SimpleOrderedMap<>(newList);
  }
+
+  public static SimpleOrderedMap<Object> emptySimpleOrderedMap() {


Needs a javadoc. And that name is quite redundant given the type where we're declaring this.
WDYT of naming this simply of thus mimicking the end effect of List.of() or Set.of() or Map.of()?

The method name of() is implying, at least to me, that something can be passed in and and creates something else out of it. Like when you do List.of("String"). Maybe empty() would be the better option. What do you thing?

Using of opens the door for some more static methods for more, which you could add now if it makes of more attractive to you. Just a single pair would be fine and perhaps useful. I look to the JDK for inspiration on data structure conventions.

ok, I see you point regarding of() vs empty(). I looked at how it is done on the List and have implemented some more of()-method for 1,2 and n-elements. what do you think?

adding java doc

dsmiley · 2025-01-10T13:58:00Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

+  /**
+   * Returns an immutable instance of SimpleOrderedMap with a single element.
+   * @return List containing the elements
+   */
+  public static SimpleOrderedMap<Object> of(Object o1) {
+    return new SimpleOrderedMap<>(List.of(o1));
+  }


This is illogical; the list should be key-value pairs. SimpleOrderedMap is fundamentally a Map. Look at Map's static "of" methods for inspiration.

dsmiley · 2025-01-10T13:59:55Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

+   * Returns an immutable instance of SimpleOrderedMap with two elements.
+   * @return List containing the elements
+   */
+  public static SimpleOrderedMap<Object> of(Object o1, Object o2) {


named key and value, thus logically one pair

of course, was confused by the fact it internally holds a List t store the values.
Do you think it still makes sense to have and of for two key-value pairs e.g. like this:
of(String name1, Object val1, String name2, Object val2)

Looking at Map.of, I think it would also make to use type inference rather than , like this:
public static SimpleOrderedMap of(String name, T val)
What do you think?

Basically, if Map has it, we can to.
Any way, I also think we shouldn't over-invest in SimpleOrderedMap without callers of these methods.

dsmiley · 2025-01-10T14:04:30Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

+   * Returns an immutable instance of SimpleOrderedMap with an arbitrary number of elements. 
+   * @return List containing the elements
+   */
+  public static SimpleOrderedMap<Object> of(Object... elements) {


Map.of has no similar array based argument so I suggest not doing the same here. Presumably the key and value distinction then becomes less obvious; easier to misuse.

…deredMap

) SimpleFacets: use this, avoiding needless new NamedList creation tidy (cherry picked from commit 6c02da2)

dsmiley · 2025-01-11T20:39:28Z

Also merged to branch_9x thus will appear in Solr 9.9.
Thanks for contributing!

dsmiley · 2025-01-16T14:37:07Z

If you're interested: https://issues.apache.org/jira/browse/SOLR-17623

renatoh · 2025-01-16T15:25:37Z

sure, i will give it a go.
so the idea is to keep the ArrayList from NamedList as internal data structure but do i implement java.util.Map, correct?
How do I get access to Jira?

dsmiley · 2025-01-16T16:03:02Z

Yes; simply add methods to SimpleOrderedMap.

JIRA: Should be in the header of the page: https://selfserve.apache.org/jira-account.html

reusing empty NamedList rather than recreating a new empty NamedList …

0e8dcca

…every time the facet.field parameter is empty

github-actions bot added client:solrj cat:search labels Jan 3, 2025

collapsing catch block

d4fc020

using single instance of empty SingleOrderedMap

ff25f8b

using immutableList internaly for empty SimpleOrderedMap

7ebc32e

dsmiley reviewed Jan 8, 2025

View reviewed changes

renatoh added 2 commits January 9, 2025 20:27

renaming instance variable

7bcb2d9

adding java doc

implementing factory-methods on SimpleOrderedMap

e1ab4a7

dsmiley reviewed Jan 10, 2025

View reviewed changes

implementing factory-methods for empty and key-value pair on SimpleOr…

206c0ac

…deredMap

dsmiley approved these changes Jan 10, 2025

View reviewed changes

dsmiley merged commit 6c02da2 into apache:main Jan 11, 2025
1 check passed

dsmiley pushed a commit that referenced this pull request Jan 11, 2025

Added SingleOrderedMap.of(...) for immutable empty or single pair (#2932

ec6dc11

) SimpleFacets: use this, avoiding needless new NamedList creation tidy (cherry picked from commit 6c02da2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reusing empty NamedList rather than recreating a new empty NamedList … #2932

reusing empty NamedList rather than recreating a new empty NamedList … #2932

renatoh commented Jan 3, 2025 •

edited

Loading

renatoh commented Jan 3, 2025

madrob commented Jan 3, 2025

renatoh commented Jan 4, 2025

madrob commented Jan 4, 2025

dsmiley commented Jan 5, 2025

renatoh commented Jan 5, 2025

dsmiley commented Jan 5, 2025

renatoh commented Jan 5, 2025

dsmiley commented Jan 6, 2025

renatoh commented Jan 6, 2025

dsmiley commented Jan 6, 2025

dsmiley Jan 8, 2025

dsmiley Jan 8, 2025

renatoh Jan 9, 2025

dsmiley Jan 8, 2025

renatoh Jan 9, 2025

dsmiley Jan 9, 2025

renatoh Jan 10, 2025

dsmiley Jan 10, 2025

dsmiley Jan 10, 2025

renatoh Jan 10, 2025 •

edited

Loading

dsmiley Jan 10, 2025

dsmiley Jan 10, 2025

dsmiley commented Jan 11, 2025

dsmiley commented Jan 16, 2025

renatoh commented Jan 16, 2025

dsmiley commented Jan 16, 2025

reusing empty NamedList rather than recreating a new empty NamedList … #2932

reusing empty NamedList rather than recreating a new empty NamedList … #2932

Conversation

renatoh commented Jan 3, 2025 • edited Loading

renatoh commented Jan 3, 2025

madrob commented Jan 3, 2025

renatoh commented Jan 4, 2025

madrob commented Jan 4, 2025

dsmiley commented Jan 5, 2025

renatoh commented Jan 5, 2025

dsmiley commented Jan 5, 2025

renatoh commented Jan 5, 2025

dsmiley commented Jan 6, 2025

renatoh commented Jan 6, 2025

dsmiley commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatoh Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley commented Jan 11, 2025

dsmiley commented Jan 16, 2025

renatoh commented Jan 16, 2025

dsmiley commented Jan 16, 2025

renatoh commented Jan 3, 2025 •

edited

Loading

renatoh Jan 10, 2025 •

edited

Loading