Arquillian tests and pull request for performance improvements
November 4, 2011 at 10:59 am #18076
what do I have to do to get the Arquillian tests to work? They don’t work out the box, which I expected. Anyway: I created a pull request for performance improvements: https://github.com/ocpsoft/prettyfaces/pull/9
Ciao FrankNovember 4, 2011 at 4:22 pm #21646
the Arquillian tests should work out of the box. Do you get an error? On my box individual tests sometimes fail without any reason but succeed when I run the test again. I’m not sure what the issue is. Could you post your error?
I saw you pull request. Thanks for creating it. However I think the patch introduces some bugs. I for example saw that you replaced the URL encoding based on java.net.URI with URLDecoder/URLEncoder. This is definitively wrong. We did this in the past and it was incorrect because these classes are not meant to be used for URL but for FORM data encoding/decoding.
I’ll take a deeper look at your patch as soon as I find some time. But I’m travelling next week so this could take some time.
ChristianNovember 8, 2011 at 1:54 pm #21647
the Arquilian tests expected 3.3.1-SNAPSHOT, but I compiled 3.3.2-SNAPSHOT. When I have more time, I’ll have a deeper look into this.
I always thought URL encoding is URL encoding. Is there a unit-test for the bug with URLEncoder/URLDecoder? Using URI for this is quite slow. Having an example, I could find a better performing solution for this.
Ciao FrankNovember 8, 2011 at 10:21 pm #21648
Lincoln Baxter IIIKeymaster
I looked at your pull request, and it looks good. Thanks! I agree, however, that we need to take a closer look at the Encoding changes and make sure that we have tests for all of the cases. I’m not sure that we have sufficient coverage, but we can find out.
We can release 3.3.3 with your patch as soon as this is done (no need for a long waiting period since the changes are all very minor.)
~LincolnNovember 10, 2011 at 7:41 pm #21649
if you like, you can release 3.3.3 without the encoding changes. We can have a look at the encoding thing later, since it was not the main problem.
Ciao FrankNovember 11, 2011 at 11:05 am #21650
Yeah, I know that using URI is not a very nice solution. I guess we should really do the URL encoding and decoding ourselves someday. But my guess is that this is getting very complicated. I’ll have a look at the corresponding spec as soon as I find some time.
ChristianJanuary 4, 2012 at 4:40 pm #21651
sorry for the delay. I finally found some time to look at your pull request.
First of all: Thank you very much for taking the time to do these performance improvements. I tried to group them into logical units and committed them individually:
However there are three changes contained in the patch which I didn’t merge because I’ve some concerns.
First you changed the code responsible for the URL encoding to use URLEncoder. Unfortunately this is not correct. Despite it’s name this class doesn’t do URL encoding but encodes data posted during form submits. The encoding used here is a bit different from the encoding for URLs. I’ve already spent some hours of my life messing with this encoding stuff and using URLEncoder would definitively break things. I know because we did this previously.
The second change is the use of HttpServletRequest.isRequestedSessionIdFromURL() to detect if the URL contains a session ID. For some reason this doesn’t seem to work. At least the integration tests are failing with this. Why did you implement this change? Do you think this brings major performance improvements compared to the regular expression test we currently do?
Another thing that I didn’t merge is the cache you implemented to store URLs that don’t match any URL pattern. I see the advantage of doing this and that this will lead to better performance especially for the enormous number of requests that are processed by PrettyFaces which aren’t mapped (like images, CSS files, etc). But the problem with this is that it makes PrettyFaces vulnerable to DOS attacks. Imagine somebody sends an enormous number of requests with random URLs to the server. Each of the requests will end up with a 404 but every request will also put an entry into the cache which will eat up memory after some time. I think this could be very dangerous. This could work if we limit the size of the cache somehow.
However. Thank you very much for your work. We really appreciate this.
PS: If you plan to submit further patches in the future it would be great if you could group the changes to smaller logical units so they are reviewable more easily. This way you could also write a short comment on each changes describing what you do and why.January 6, 2012 at 7:04 pm #21652
Lincoln Baxter IIIKeymaster
Thanks so much for looking in to this. I had a thought about caching. It would be nice if we had a MappingCacheProvider that would enable pluggable custom caching for this feature.
Do you think that would let you do what you need? We could then, from there, explore a variety of caching options, and possibly provide some options out of the box in PrettyFaces.
~LincolnJanuary 23, 2012 at 9:00 am #21653
sorry for answering that late. I’m quite busy currently.
I think a MappingCacheProvider is nice, but a little bit too heavy. A simple LRU cache with a configurable size should suite all fits. You should also have the option to disable caching completely.
I think caching is essential, because PrettyFaces is made for SEO and good SEO results in high traffic.
I wonder why HttpServletRequest.isRequestedSessionIdFromURL() doesn’t work as expected. Regular expressions are slow, especially, when they are reconstructed on every use instead of simply reusing them (java.util.regex.Pattern is thread safe). This is why I tried to eleminate them.
I will have a look at HttpServletRequest.isRequestedSessionIdFromURL() and the URLEncoder thing later.
Ciao FrankJanuary 23, 2012 at 6:25 pm #21654
Yeah, a cache would be a good idea I think. We should create a ticket to track this. Could you do this? Then you will be notified if we implement something!
I don’t know why isRequestedSessionIdFromURL() isn’t working. But AFAIK the pattern is now precompiled, so I don’t think that it has a major performance impact any more.February 8, 2012 at 6:39 pm #21655
The issue is created: https://github.com/ocpsoft/prettyfaces/issues/13
The pattern is not precompiled because you don’t use Pattern.
requestUrl = requestUrl.replaceFirst(JSESSIONID_REGEX, JSESSIONID_REPLACEMENT);
Here the pattern is compiled twice, if it matches.
Ciao FrankFebruary 12, 2012 at 2:57 pm #21656
Yeah, I guess you are right. This should really be fixed.
Could you open another ticket for this?
ThanksFebruary 13, 2012 at 4:39 pm #21657
No need to create a ticket. I just committed a fix for this:
You must be logged in to reply to this topic.