Improve MultiValueTable #829

venfernand · 2023-02-12T11:55:17Z

MultiValueTable previously used HashTable class internally and all access methods required to be synchronized. This solution was replaced with ConcurrentHashMap and CopyOnWriteArrayList, which may be accessed concurrently and with less synchronization.

Also changed some methods, which used older collection APIs.

Improved usages of MultiValueTable.

Signed-off-by: Veniamin Fernandes [email protected]

MultiValueTable previously used HashTable class internally and all access methods required to be synchronized. This solution was replaced with ConcurrentHashMap and CopyOnWriteArrayList, which may be accessed concurrently and with less synchronization. Also changed some methods, which used older collection APIs. Improved usages of MultiValueTable. Signed-off-by: Veniamin Fernandes <[email protected]>

src/freenet/clients/fcp/RequestStatusCache.java

ArneBab · 2023-03-27T05:16:46Z

src/freenet/clients/fcp/RequestStatusCache.java

+			}
+			if (noFilter && download.filterData) {
+				continue;
+			}


nice change! (we need this at many places — but only where we actually change stuff to avoid breaking existing pull requests we cannot merge yet)

ArneBab · 2023-03-27T05:17:45Z

src/freenet/clients/http/ConfigToadlet.java

+			MultiValueTable<String, String> headers = MultiValueTable.from(
+				"Location",
+				directoryBrowserPath + params + "path=" + core.getDownloadsDir().getAbsolutePath()
+			);


much nicer — thank you!

src/freenet/support/MultiValueTable.java

ArneBab · 2023-04-12T07:38:05Z

@venfernand thank you for your pull request! This makes the code much nicer.

To ensure that this does not break plugins, it needs to preserve all existing API, so to be able to merge it, this needs shims for all previously existing public methods and constructors.

ArneBab · 2023-05-23T06:15:32Z

@venfernand will you add the shims to preserve the existing API?

Mark some previous methods as deprecated.

venfernand · 2023-10-08T08:15:15Z

@venfernand will you add the shims to preserve the existing API?

Sorry for being too distructive.
I restored all previous public API in MultiValueTable and marked some methods as @Deprecated to speed up transition to newer methods.

src/freenet/clients/http/ContentFilterToadlet.java

src/freenet/clients/http/FProxyFetchTracker.java

Bombe · 2023-10-15T10:33:22Z

src/freenet/clients/http/FProxyFetchTracker.java

 						return progress;
-					} else
-						if(logMINOR) Logger.minor(this, "Skipping "+progress);
+					} else if (logMINOR) {


As the first if branch ends with a return, we don’t need the else here and get rid of that one level of indentation.

I tried to improve this method even more.

Bombe · 2023-10-15T10:56:07Z

src/freenet/clients/http/ToadletContextImpl.java

-import java.util.Locale;
-import java.util.StringJoiner;
-import java.util.TimeZone;
+import java.util.*;


Please don’t use star imports, they can make this code break in the future.

Bombe · 2023-10-15T11:07:35Z

src/freenet/clients/http/ToadletContextImpl.java

 		}
+
+		// content may be cached
+		// For privacy reasons, only static


Comments in the wrong order. 😄 (Also, put them into one line, that’s toally fine.)

I think it was an outcome of some automatic refactoring or just regular blindness :)

Bombe · 2023-10-15T11:40:22Z

src/freenet/support/MultiValueTable.java

+        removeAndGet(key);
+    }
+
+    public List<V> removeAndGet(K key) {


Do we actually need this method? For the same reason that we can’t simply remove methods we should also be wary about adding new public methods, so if we don’t have an immediate use for it elsewhere in Fred, maybe add this as private first, if at all?

I may be used when atomic remove and retrieve is required, but you are right, there are no real usages at the moment.
I may be added quite easily if this will be required.
I removed it.

Bombe · 2023-10-15T11:47:39Z

src/freenet/support/MultiValueTable.java

-        public final boolean hasMoreElements() {
-            return global.hasMoreElements(); // || current.hasMoreElements();
-        }
+    public Stream<V> values() {


I don’t think this method should return a Stream, mostly because it seems to be rare for collections to return a Stream from anywhere else but a .stream() method. Also, if this returned a collection, you could make the elements() method way shorter. :)

I tried to return a lazy view, which does not allocate that much.
But i agree that this breaks expectations of callers and it is better to return a simpler type from there.
I changed it to return Collection<V> and now it looks simpler.

Bombe · 2023-10-15T13:38:36Z

test/freenet/support/MultiValueTableTest.java

Oof, most of these tests are pretty horrible. 🙁 Coverage is pretty good, though, so they’ll do.

Do you mean that there is no need to check such trivial places?
Or those preparations of test collections are not very clear?

…ariable is declared

Invert if condition with null check and remove one level of indentation. Add logging for the case when fetch context does not match. Improve the Javadoc.

This is a new method, which was added recently. There is no use case for it a the moment. It can be added back again if necessary.

Previous solution with java.util.stream.Stream used to return a lazy view of MultiValueTable values. Now this method returns a Collection, which makes it to be similar to java.util.Map.values().

venfernand · 2023-10-20T20:30:26Z

I added some more fixes.

bertm · 2023-10-20T20:48:34Z

src/freenet/support/MultiValueTable.java

+        });
+    }
+
+    public void putAll(K key, Collection<V> elements) {


Collection<V> put arguments should read Collection<? extends V> instead, so it becomes possible to put a List<Foo> into a MultiValueTable<?, Object>.

Pushed changes for putAll() and from() methods.

bertm · 2023-10-20T21:05:28Z

src/freenet/support/MultiValueTable.java

-                v.elements());
+        List<V> elements = getAllAsList(key);
+        if (elements.isEmpty()) {
+            return Collections.emptyEnumeration();


This special case is unnecessary. An enumeration over an empty collection is equivalent to an empty enumeration.

The only benefit of Collections.emptyEnumeration() is that it returns same object instance all the time.
Otherwise, yes, it can be simpler.
Will make it to be one-liner then.

bertm · 2023-10-20T21:09:15Z

src/freenet/support/MultiValueTable.java

+    /**
+     * Returns the first element for this key.
+     */
+    public V getFirst(K key) {


Now this is just a duplicate of get. Is there any real need for extending the public API with this method? If so, please consider deprecating the method it replaces.

New method looks more explicit for me.
Besides that there is no that big need and old method may be still used.
Which variant do you prefer more?

getFirst make way more sense indeed (not that any of the other methods of this class make much sense, but that's all hindsight)

We should not have 2 non-deprecated methods doing the exact same thing, because then things get confusing for developers - which one should you use? I'm all for getFirst, but then let's deprecate get and refer to getFirst in the @deprecated notice.

Added more deprecation comments there.

bertm · 2023-10-20T21:18:17Z

src/freenet/support/MultiValueTable.java

-                table.put(key, v);
+        this.table.compute(key, (k, list) -> {
+            if (list == null) {
+                list = new CopyOnWriteArrayList<>();


Why the CoW list? If we stick to unmodifiable lists instead (replace the list in the map, instead of altering its contents), the implementation becomes a whole lot easier to reason about (once you've read a value list, its contents remain stable forever) and you don't need to mess with wrapping in Collections.unmodifiableList() on every read.

CopyOnWriteArrayList was picked to allow concurrent iteration and access to be safe.
It also was a continuation of the previous implementation, where underlying data structure was modified in place.

Immutable lists are really better here, but these require some more code to write.
Stable collections may also prevent from various concurrent modification bus.

I pushed new changes containing immutable lists instead of CopyOnWriteArrayList.

bertm · 2023-10-20T21:23:23Z

src/freenet/support/MultiValueTable.java

+        if (list == null || list.isEmpty()) {
+            return Collections.emptyList();
+        }
+        return Collections.unmodifiableList(list);


This returns an unmodifiable read-through view of the underlying list. This means that when a new entry is inserted or an existing entry deleted in the table for this key, the change is reflected in this returned list. That is highly unexpected (and if intended, should be thoroughly documented), most likely undesired, and probably an unintended effect of choice of datastructure. See also my comment about the use of CoWList.

This was also true for the Enumeration objects, which were returned before.
I think that nobody ever used this like that and wanted any updates during iteration or later use.
I agree that it is really better to return "safe" and stable collections and iterators.

Now, when lists are truly immutable, then all returned values are also not modifiable.
Please see the new changes, it is probably better now.

bertm · 2023-10-20T21:37:14Z

src/freenet/support/MultiValueTable.java

+        List<V> list = this.table.get(key);
+        if (list == null || list.isEmpty()) {
+            return Collections.emptyList();
+        }


This could be reduced to a simple oneliner:

List<V> list = table.getOrDefault(key, Collections.emptyList());

Yes, the new implementation looks exactly like that.
When table holds immutable lists, then those may be returned as is.

bertm · 2023-10-20T22:17:23Z

src/freenet/clients/fcp/RequestStatusCache.java

+		if (downloads == null) {
+			return null;
+		}


Unreachable return, getAllAsList never returns null

bertm · 2023-10-20T22:18:56Z

src/freenet/clients/http/FProxyFetchTracker.java

+			if (fetchList == null) {
+				return null;
+			}


Unreachable return; fetchList is never null.

bertm · 2023-10-20T22:25:06Z

src/freenet/support/MultiValueTable.java

-                return r;
-            }
-        }
+        return getAllAsList(key).toArray();


This breaks the (undocumented) return value semantics of getArray: previously it would return null when no values are present, currently it returns an empty array.

Please consider simulating the old behaviour and documenting it in the Javadoc.

Restored the previous behavior and put notes into Javadoc

src/freenet/support/MultiValueTable.java

bertm · 2023-10-20T22:45:49Z

src/freenet/support/MultiValueTable.java

-		synchronized (table) {
-			return table.get(key);
-		}
+        return getAllAsList(key);


This method used to return a Vector (or null if there was no value), now it always returns a List. There are many methods that are implemented by Vector but not by List, so this is a breaking change (even though this behaviour was undocumented, as usual).

Additionally, this now returns an unmodifiable list where it would previously allow modifications. We might as well remove this method right away - we cannot in any reasonable way simulate previous behaviour using this new implementation, and no sane plugin should use it anyways (given its entirely undocumented nature).

I checked the following plugins to be sure (listing by Plugin-Main-Class):

de.saces.fnplugins.Shoeshop.ShoeshopPlugin de.saces.fnplugins.SiteToolPlugin.SiteToolPlugin freenetsearch.Plugin keepalive.Plugin net.pterodactylus.sone.main.SonePlugin org.freenetproject.freemail.FreemailPlugin plugins.CENO.Client.CENOClient plugins.floghelper.FlogHelper plugins.flophelper.FlogHelper plugins.FMS.FMSPlugin plugins.Freereader.Freereader plugins.JSTUN.JSTUN plugins.KeyUtils.KeyUtilsPlugin plugins.Library.Main plugins.MDNSDiscovery.MDNSDiscovery plugins.N2NChat.core.N2NChatPlugin plugins.ShareLink.Plugin plugins.Sharesite.Plugin plugins.ShareWiki.Plugin plugins.ThawIndexBrowser.ThawIndexBrowser plugins.UPnP2.UPnP2 plugins.UPnP.UPnP plugins.WebOfTrust.WebOfTrust

None of them use anything from MultiValueTable except:

freenet.support.MultiValueTable#<init>() // the no-args constructor freenet.support.MultiValueTable#put(java.lang.Object,java.lang.Object)

@ArneBab @Bombe thoughts on removing this method?

I changed this back to return Vector.
It may be removed at any time later.

bertm · 2023-10-20T23:00:05Z

src/freenet/support/MultiValueTable.java

+     * @param key key mapping
+     */
+    @Deprecated
    public Enumeration<K> keys() {


This method does not have a key parameter even though one is specified in the Javadoc.

Co-authored-by: Bert Massop <[email protected]>

Previous solution containing CopyOnWriteArrayList had redundant Collections.unmodifiableList() wrapping for each read in getAllAsList(). It is better to make the underlying list to be immutable. This also provides stable results for callers if MultiValueTable is modified after elements are already returned to caller.

…ment their behavior Wrapped returned values from entrySet() and keySet() with unmodifiable views. Described more verbosely the properties of returned values.

This method previously used to return null when key is absent.

…ector from there

ArneBab · 2024-11-30T10:30:08Z

@bertm is this ready to be merged, now?

@venfernand can you merge next into your branch and resolve the conlicts?

bertm

There is still room for improvement, but this looks good enough to go.

src/freenet/support/MultiValueTable.java

bertm · 2024-11-30T11:03:49Z

src/freenet/support/MultiValueTable.java

+            List<V> result;
+            if (previousList == null) {
+                // FIXME: replace with List.of(v) when Java version baseline becomes >= 11
+                result = new ArrayList<>(1);


This could be optimized to return Collections.singletonList(value);.

bertm · 2024-11-30T11:23:41Z

src/freenet/support/MultiValueTable.java

+        this.table.compute(key, (k, previousList) -> {
+            List<V> result;
+            if (previousList == null) {
+                result = new ArrayList<>(elements.size());


This could be optimized to return new ArrayList<>(elements);.

ArneBab · 2024-11-30T14:39:11Z

merged after resolving conflicts. Waiting longer would only increase the amount of conflicts.

ArneBab · 2024-11-30T14:39:22Z

@venfernand thank you for your PR!

ArneBab reviewed Apr 10, 2023

View reviewed changes

Merge branch 'next' into fred-multi-value-table-improvement

573cee3

venfernand added 3 commits October 8, 2023 10:37

restore previous public API of freenet/support/MultiValueTable.java

00f9ef2

Mark some previous methods as deprecated.

Merge branch 'next' into fred-multi-value-table-improvement

0525735

fix code indentation in freenet/support/MultiValueTable.java

ab7e17c

venfernand requested a review from ArneBab October 8, 2023 08:15

Bombe requested changes Oct 15, 2023

View reviewed changes

venfernand added 8 commits October 20, 2023 19:36

Merge branch 'next' into fred-multi-value-table-improvement

a9aae2d

Fix comment in ToadletContextImpl.sendReplyHeaders()

0a32278

Expand star imports in ToadletContextImpl

7db50fd

Move assignment of FProxyFetchTracker.fetchers into the place where v…

3bfbc44

…ariable is declared

Remove unused import from FProxyFetchTracker

c6f8d7f

Improve method FProxyFetchTracker.getFetchInProgress()

3b5523d

Invert if condition with null check and remove one level of indentation. Add logging for the case when fetch context does not match. Improve the Javadoc.

Remove the MultiValueTable.removeAndGet() method

2eef2a0

This is a new method, which was added recently. There is no use case for it a the moment. It can be added back again if necessary.

Return a Collection from MultiValueTable.values()

7f8ef91

Previous solution with java.util.stream.Stream used to return a lazy view of MultiValueTable values. Now this method returns a Collection, which makes it to be similar to java.util.Map.values().

venfernand requested a review from Bombe October 20, 2023 20:30

bertm reviewed Oct 20, 2023

View reviewed changes

Make MultiValueTable.putAll() to accept sub-types

2d85389

bertm reviewed Oct 20, 2023

View reviewed changes

src/freenet/support/MultiValueTable.java Outdated Show resolved Hide resolved

bertm reviewed Oct 20, 2023

View reviewed changes

venfernand and others added 8 commits October 21, 2023 03:00

Change deprecation note in MultiValueTable.getArray() method Javadoc

5573a5f

Co-authored-by: Bert Massop <[email protected]>

Improve immutability of values returned from MultiValueTable and docu…

98e1cf7

…ment their behavior Wrapped returned values from entrySet() and keySet() with unmodifiable views. Described more verbosely the properties of returned values.

Add deprecation notes for MultiValueTable.get(key) method

7fed29f

Restore previous behavior of MultiValueTable.getArray() method

2217f1f

This method previously used to return null when key is absent.

Restore previous behavior of MultiValueTable.getSync() and return a V…

f7d34e3

…ector from there

Remove redundant null check from RequestStatusCache.getShadowBucket()

117dad4

Remove redundant null check from FProxyFetchTracker.getFetchInProgress()

e7ff0e4

bertm approved these changes Nov 30, 2024

View reviewed changes

ArneBab added 3 commits November 30, 2024 15:23

Merge branch 'next' into fred-multi-value-table-improvement

b79a2a0

Fix merge error

36cd30b

Fix merge error

643cbee

ArneBab merged commit 1b1c950 into hyphanet:next Nov 30, 2024
1 check passed

Improve MultiValueTable #829

Improve MultiValueTable #829

Conversation

venfernand commented Feb 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArneBab commented Apr 12, 2023

ArneBab commented May 23, 2023

venfernand commented Oct 8, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venfernand commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertm Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venfernand Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertm Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertm Oct 20, 2023 • edited Loading

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

bertm Oct 20, 2023 • edited Loading

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

Choose a reason for hiding this comment

ArneBab commented Nov 30, 2024

bertm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArneBab commented Nov 30, 2024

ArneBab commented Nov 30, 2024

bertm Oct 20, 2023 •

edited

Loading

venfernand Oct 20, 2023 •

edited

Loading

bertm Oct 20, 2023 •

edited

Loading

bertm Oct 20, 2023 •

edited

Loading

bertm Oct 20, 2023 •

edited

Loading