Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tries cleanup #5566

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Tries cleanup #5566

merged 1 commit into from
Nov 26, 2020

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Nov 3, 2020

Replace all Trie usages in the code base with a more abstract hierarchy of *Index interfaces, and do not directly expose the implementations, requiring the use of a factory exposed via the builder pattern to create instances.

This should help maintenance as the implementation will be chosen based on a set of requirements (case-sensitivity, mutability...) that the factory can interpret as it wants to provide the best implementation complying with them.

Clears the path for a proper fix for #5291

@lorban
Copy link
Contributor Author

lorban commented Nov 11, 2020

I've now removed all references to the Trie interface and its implementations across the code base. Three things are now left, in order:

  1. Fix some bugs in ArrayTrie (there seems to be some off-by-1 errors making the tests fail).
  2. Finalize the *Index interfaces.
  3. Seriously ponder the Builder interfaces, maybe merge them all into a single one. The content initializing methods (with, withAll) and the config methods (caseSensitive(), fixedCapacity()) deserve a good dose of brain juice, especially the latter's defaults.
  4. Clean up the implementations.

@lorban
Copy link
Contributor Author

lorban commented Nov 24, 2020

@lachlan-roberts I've added you as a reviewer to get your opinion about the look and feel of the Index, Index.Builder and MutableIndex API. Don't bother with the implementations as they're just a big hack at the moment so please just focus on how elegant or clunky it is to create and use indices.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!
Can we push on this to get it in before a .0.0
Even if the impls are not perfect, they are the same as we had before and the Builder abstraction would be good to get in for a .0.0

@lorban lorban marked this pull request as ready for review November 25, 2020 11:47
Comment on lines 1076 to 1082
if (_field == null)
{
HttpField field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString);
if (_fieldCache.put(field))
_field = field;
}
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm not having isFull changes this a bit... plus I;m not sure it is right. Maybe something like:

Suggested change
if (_field == null)
{
HttpField field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString);
if (_fieldCache.put(field))
_field = field;
}
else
if (_field == null)
_field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString);
if (!_fieldCache.put(field))
{
_fieldCache.clear();
_fieldCache.put(field);
}

Copy link
Contributor Author

@lorban lorban Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we should rely on some eviction algorithm in the index instead of clearing it. But that's quite a can of worm that should be left closed if possible.

Let's use that if (!put) clear & put logic, it should be roughly equivalent to what the old code is doing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think before it just filled up the cache and left it as is.... so maybe we just put and ignore the return for now.... we can refine in future PRs

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No show stoppers for me now... but leave the issue open so we can refine a bit after .0.0

@lorban lorban requested a review from gregw November 26, 2020 10:36
… Builders

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban merged commit 981e263 into jetty:jetty-10.0.x Nov 26, 2020
@lorban lorban deleted the tries-cleanup branch November 26, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants