jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.86k stars 1.91k forks source link

Avoid list copy on reverse iteration #12297

Closed gregw closed 1 month ago

gregw commented 1 month ago

The changes in #12289 introduced several list copy and reverses operations on every request. This PR replaces that reversed list with a ListIterator used with previous

gregw commented 1 month ago

@sbordet @lorban an alternative approach would be to have the following method:


    /**
     * <p>Returns an {@link Iterator} that efficiently iterates the passed list in reverse.</p>
     * <p>The method is safe for concurrent modifications iff the list iterator itself is safe.</p>
     * @param list the list
     * @param <T> the element type
     * @return An {@link Iterator} over the list elements in reverse order
     * at the snapshot of the list represented by this call.
     */
    public static <T> Iterator<T> reversed(List<T> list)
    {
        ListIterator<T> i;
        try
        {
            int size = list.size();
            switch (size)
            {
                case 0:
                    return Collections.emptyIterator();
                case 1:
                    return list.iterator();
                default:
                    i = list.listIterator(size);
            }
        }
        catch (IndexOutOfBoundsException e)
        {
            // list was concurrently modified, so do this the hard way
            i = list.listIterator();
            while (i.hasNext())
                i.next();
        }

        ListIterator<T> iterator = i;
        return new Iterator<T>()
        {
            @Override
            public boolean hasNext()
            {
                return iterator.hasPrevious();
            }

            @Override
            public T next()
            {
                return iterator.previous();
            }
        };
    }
sbordet commented 1 month ago

@lorban but that's another allocation.

lorban commented 1 month ago

@sbordet possibly, but I sometimes feel we're a bit too obsessed with allocations so I'm tempted to spend some time looking at the JIT's efficiency at generating stack allocating code to deny/confirm if our fears are justified.

In all case, I think exposing an iterator that delegates to ListIterator's previous() and hasPrevious() like @gregw suggested in his alternative would still look better IMHO.

joakime commented 1 month ago

Please fix the title of this PR, so it makes sense on a changelog.

gregw commented 1 month ago

Instead of modifying the iterations, we could keep TypeUtil.reverse() and implement it this way:

The resulting list would have an list with an iterator that does not match the get(int) behaviour. If that list ever got out into the wild, then chaos would reign.

However, I'm not totally opposed to my alternative, even though that is an extra allocation.... The resulting code is a little neater.... but then it pretty simple code anyway, and seldom touched, and clear what it does, so in this case the extra allocation doesn't get us much.