Arquillian tests and pull request for performance improvements

Splash Forums PrettyFaces Users Arquillian tests and pull request for performance improvements

This topic contains 12 replies, has 3 voices, and was last updated by  Christian Kaltepoth 5 years, 7 months ago.

Viewing 13 posts - 1 through 13 (of 13 total)
  • Author
    Posts
  • #18076

    Frank Caputo
    Participant

    Hi,

    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 Frank

    #21646

    Hey Frank,

    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.

    Best regards

    Christian

    #21647

    Frank Caputo
    Participant

    Hi Christian,

    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 Frank

    #21648

    Frank,

    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.)

    ~Lincoln

    #21649

    Frank Caputo
    Participant

    Hi Lincoln,

    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 Frank

    #21650

    @Frank:

    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.

    Christian

    #21651

    Hey Frank,

    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:

    https://github.com/ocpsoft/prettyfaces/commit/94768307e7e0cc0c775443d16f83d92bc97b2d0f

    https://github.com/ocpsoft/prettyfaces/commit/c8dfbca093737228d4e122dd069bfa0074b7dded

    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.

    Christian

    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.

    #21652

    Christian,

    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.

    Frank,

    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.

    ~Lincoln

    #21653

    Frank Caputo
    Participant

    Hi,

    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 Frank

    #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.

    #21655

    Frank Caputo
    Participant

    The issue is created: https://github.com/ocpsoft/prettyfaces/issues/13

    The pattern is not precompiled because you don’t use Pattern.

    if (requestUrl.matches(JSESSIONID_REGEX))

    {

    requestUrl = requestUrl.replaceFirst(JSESSIONID_REGEX, JSESSIONID_REPLACEMENT);

    }

    Here the pattern is compiled twice, if it matches.

    Ciao Frank

    #21656

    Yeah, I guess you are right. This should really be fixed.

    Could you open another ticket for this?

    Thanks

    #21657
Viewing 13 posts - 1 through 13 (of 13 total)

You must be logged in to reply to this topic.

Comments are closed.