Issue #4861 - reduce garbage created by the async request attributes #4867
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Lachlan Roberts <[email protected]>
gregw
requested changes
May 12, 2020
Comment on lines
26
to
33
| public class ServletAttributes implements Attributes | ||
| { | ||
| private final Attributes _attributes; | ||
|
|
||
| public ServletAttributes() | ||
| { | ||
| _attributes = new AsyncAttributes(new AttributesMap()); | ||
| } |
There was a problem hiding this comment.
I don't like all the dereferences here. We end up with ServletAttributes->AsyncAttributes->AttributesMap, so 3 object and 3 dereferences.
I think it would be much better to do (also note the name change):
Suggested change
| public class ServletAttributes implements Attributes | |
| { | |
| private final Attributes _attributes; | |
| public ServletAttributes() | |
| { | |
| _attributes = new AsyncAttributes(new AttributesMap()); | |
| } | |
| public class AsyncAttributesMap extends AttributesMap | |
| { | |
| public AsyncAttributesMap() | |
| { | |
| } |
The override the methods as follows:
@Override
 public Object getAttribute(String name)
 {
// do the AsyncAttribute handling here
// else
return super.getAttribute(name);
}Then you don't need AsyncAttributes class.
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncAttributes.java
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncAttributes.java
Show resolved
Hide resolved
gregw
requested changes
May 12, 2020
| @@ -199,7 +200,7 @@ public static Request getBaseRequest(ServletRequest request) | |||
| private boolean _handled = false; | |||
| private boolean _contentParamsExtracted; | |||
| private boolean _requestedSessionIdFromCookie = false; | |||
| private Attributes _attributes; | |||
| private Attributes _attributes = new ServletAttributes(); | |||
There was a problem hiding this comment.
Ah we can't make this final because of possible wrappers that don't unwrap.
So go back to lazy creation of this field...... OR have 2 fields:
Suggested change
| private Attributes _attributes = new ServletAttributes(); | |
| private final AsyncAttributesMap _asyncAttributes = new AsyncAttributesMap(); | |
| private Attributes _attributes = _asyncAttributes; |
This way, async could as the request directly for the _asyncAttributes and set the fields on it.... but we'd still create an extra class with 6 fields even if we never use attributes.... hmmmm @lorban your thoughts on which is best?
There was a problem hiding this comment.
I would leave the _attributes field non-final, this is the simplest way to go.
Any sort of cleanup seem to require a non-negligible amount of refactoring, so I'd keep #4861 open until the code is cleaned up. Regarding perf, this small change has 99% of the benefits of a larger refactoring.
Signed-off-by: Lachlan Roberts <[email protected]>
gregw
approved these changes
May 13, 2020
| if (ServletAttributes.class.equals(_attributes.getClass())) | ||
| _attributes.clearAttributes(); | ||
| else | ||
| _attributes = new ServletAttributes(); |
Comment on lines
39
to
42
| if (_asyncAttributes == null) | ||
| _attributes.removeAttribute(name); | ||
| else | ||
| _asyncAttributes.removeAttribute(name); |
There was a problem hiding this comment.
I think it is clearer as:
Suggested change
| if (_asyncAttributes == null) | |
| _attributes.removeAttribute(name); | |
| else | |
| _asyncAttributes.removeAttribute(name); | |
| (_asyncAttributes == null ? _asyncAttributes : _attributes).removeAttribute(name); |
or maybe even with a private method:
attributes().removeAttribute(name);same for other methods below.
Signed-off-by: Lachlan Roberts <[email protected]>
|
Build failure is a StackOverflowError |
|
Test failure in |
|
A slightly different one of the last failure. |
…ributesMap Signed-off-by: Lachlan Roberts <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #4861
Reduce garbage produced by creating the
ConcurrentHashMapofAttributesMapwhen going async. InRequestwe now useServletAttributesas the intialAttributesinstance which can still be subsequently wrapped by forwards and includes, this will store the async mapping attributes as local variables instead of using theAttributesMap.The
HttpServletMappingneeds to be added back toAsyncAttributeswhen this is merged to 10.