Issue 3015041: Save the base Uri for a document in the AbsolutePathReferenceVisitor (Closed)
|
Created:
15 years, 1 month ago by Kuntal Loya Modified:
15 years, 1 month ago Reviewers:
dev, dev-remailer, gagan.goku CC:
cool-shindig-committers_googlegroups.com, anupama.dutta Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionCurrently, the base uri is recomputed for every Uri (node) which the AbsolutePathReferenceVisitor attempts to absolutify in a document.
Saving a reference to the once computed base uri in the Visitor will save the overhead of computing it for other nodes.
The Visitor should now be created afresh for every document content to be rewritten (in makeVisitors).
On our candidate set of html pages, we saw latency improvement of ~30% with this change.
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use ImmutableList #
MessagesTotal messages: 9
LGTM++ Please also mention the improvement in latency you got with this change. Also, if possible, please analyze the document.setUserData(baseUri) approach. my guess is it won't give observable difference, but u'l reduce the memory footprint by not creating multiple copies of AbsolutePathReferenceVisitor. http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java (right): http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:40: return Arrays.<DomWalker.Visitor>asList( could you rewrite it as ImmutableList.of(new AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)) http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:41: new AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)); could you rewrite it as ImmutableList.of(new AbsolutePathReferenceVisitor http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java (right): http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:75: // The base Uri used to absolutify relative Uris in the document being visited. absolutify relative Uris -> absolutify relative uri's
Sign in to reply to this message.
http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java (right): http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:40: return Arrays.<DomWalker.Visitor>asList( On 2010/11/10 15:14:39, gagan.goku wrote: > could you rewrite it as ImmutableList.of(new > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)) Done. http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:41: new AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)); On 2010/11/10 15:14:39, gagan.goku wrote: > could you rewrite it as ImmutableList.of(new AbsolutePathReferenceVisitor Done. http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java (right): http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:75: // The base Uri used to absolutify relative Uris in the document being visited. On 2010/11/10 15:14:39, gagan.goku wrote: > absolutify relative Uris -> absolutify relative uri's Updated the case, no apostrophe.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
Looks nice, send out to dev@ On Thu, Nov 11, 2010 at 12:39 PM, <kuntal.loya@gmail.com> wrote: > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:40: > return Arrays.<DomWalker.Visitor>asList( > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)) > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:41: > > new > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)); > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > AbsolutePathReferenceVisitor > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:75: > > // The base Uri used to absolutify relative Uris in the document being > visited. > On 2010/11/10 15:14:39, gagan.goku wrote: > >> absolutify relative Uris -> absolutify relative uri's >> > > Updated the case, no apostrophe. > > > http://codereview.appspot.com/3015041/ >
Sign in to reply to this message.
Looks nice, send out to dev@ On Thu, Nov 11, 2010 at 12:39 PM, <kuntal.loya@gmail.com> wrote: > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:40: > return Arrays.<DomWalker.Visitor>asList( > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)) > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:41: > > new > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)); > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > AbsolutePathReferenceVisitor > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:75: > > // The base Uri used to absolutify relative Uris in the document being > visited. > On 2010/11/10 15:14:39, gagan.goku wrote: > >> absolutify relative Uris -> absolutify relative uri's >> > > Updated the case, no apostrophe. > > > http://codereview.appspot.com/3015041/ >
Sign in to reply to this message.
Looks nice, send out to dev@ On Thu, Nov 11, 2010 at 12:39 PM, <kuntal.loya@gmail.com> wrote: > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:40: > return Arrays.<DomWalker.Visitor>asList( > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)) > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:41: > > new > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)); > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > AbsolutePathReferenceVisitor > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:75: > > // The base Uri used to absolutify relative Uris in the document being > visited. > On 2010/11/10 15:14:39, gagan.goku wrote: > >> absolutify relative Uris -> absolutify relative uri's >> > > Updated the case, no apostrophe. > > > http://codereview.appspot.com/3015041/ >
Sign in to reply to this message.
Looks nice, send out to dev@ On Thu, Nov 11, 2010 at 12:39 PM, <kuntal.loya@gmail.com> wrote: > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:40: > return Arrays.<DomWalker.Visitor>asList( > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)) > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java:41: > > new > AbsolutePathReferenceVisitor(AbsolutePathReferenceVisitor.Tags.RESOURCES)); > On 2010/11/10 15:14:39, gagan.goku wrote: > >> could you rewrite it as ImmutableList.of(new >> > AbsolutePathReferenceVisitor > > Done. > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java > (right): > > > http://codereview.appspot.com/3015041/diff/1/java/gadgets/src/main/java/org/a... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:75: > > // The base Uri used to absolutify relative Uris in the document being > visited. > On 2010/11/10 15:14:39, gagan.goku wrote: > >> absolutify relative Uris -> absolutify relative uri's >> > > Updated the case, no apostrophe. > > > http://codereview.appspot.com/3015041/ >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
