Uploaded image for project: 'Blossom'
  1. Blossom
  2. BLOSSOM-130

Nesting the same JSP + Performance impact

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Neutral Neutral
    • 2.0.2, 3.0
    • 2.0.1, 3.0
    • None
    • None

      The problem I'm going to describe is a little complex and seems to have enough potential to cause you some headache just to understand what's going on, not to mention any attempts to fix it safely.
      It's unlikely you'll understand it in just a few minutes, but please try to dig in it, it seems to point to a notable performance implication at the very least, and a functional bug in any case.

      The original reason why I investigated on this problem was that I usually want to use the same JSP for most of my areas (a 3-liner that simply iterates over all components in the area and displays them one by one).
      However each time I try this kind of thing, embedding the file named componentIterator.jsp twice in the hierarchy of components and areas, I get an exception saying something like this:

      javax.servlet.ServletException: File "/startPageSlider" not found.

      This is the functional bug I mentioned above.
      I spent hours trying to figure out how Blossom is even getting the idea to look for a file named "/startPageSlider" which is the request mapping of the component containing the area and shouldn't have much to do with the path of the JSP rendering the area. And even if it did have anything to do with it, ... why is Blossom trying to look for an actual file there, and why only under these conditions (two nested areas using the same JSP)?

      While investigating on this I soon came across the class

      info.magnolia.module.blossom.support.SpecialAttributeRequestWrapper

      which seems to be the culprit in at least two ways.

      The main problem that became apparent and which I'm going to mention first is that the two methods getAttribute() and shouldMasquerade() create huge recursion stacks in real life because they call each other back and forth all the time. Granted, shouldMasquerade() calls the super version, so it seems harmless at first glance, but since that one in turn calls the getAttribute() of the parent request object, which is usually another instance of SpecialAttributeRequestWrapper, the result is a huge "explosion" of requests from getAttribute() to shouldMasquerade() and from shouldMasquerade() back to getAttribute() and so on. This effect is even aggravated by two factors:
      1. Even when shouldMasquarade() has already determined the result of a super.getAttribute(xxx), the calling getAttribute(xxx) calls super.getAttribute(xxx) again, causing another cycle through a dozen to a couple of hundreds of calls of getAttribute() and shouldMasquarade() back and forth.
      2. shouldMasquarade() doesn't just check the attribute at hand for modifications but ALL attributes, causing up to 15 calls to getAttribute() from each single call to shouldMasquarade(). (I'm not sure this in itself is intended because shouldMasquarade() isn't using its method argument, which it probably should do.)

      To verify what I'm saying, please do the following:
      1. Create some simple hierarchy of areas and components, something like this:
      page (p.jsp) -> area (a1.jsp) -> component (c1.jsp) -> area (a2.jsp) -> component (c2.jsp) -> area (a3.jsp) -> component (c3.jsp)
      and create a page where this structure is populated with actual instances.
      (Two or three component levels are sufficient to demonstrate the problem, but it'll really explode if you have more than three levels since the problem has exponential complexity.)
      2. Now just create some simple log statements in the source code to log how many times you're iterating over the specialNames in class SpecialAttributeRequestWrapper within just one page request, i.e. insert a log statement in getAttribute() and one in shouldMasquarade().
      3. Then run the page and watch the log.
      4. You will be surprised by the hundreds of iterations/recursive calls there, given this simple page configuration. And you might note you're just computing the same values again and again, visiting the same recursion branches again and again.

      The performance implications should be obvious, it's a huge waste of CPU like this. And it makes it really hard to understand, to debug, and to maintain, too.

      I tried to find out WHY the class was coded the way it is, and to be frank, I still have no idea. It just doesn't seem to make any sense to me, and the behaviour at hand suggests that some error in reasoning was involved when coding it like it is, because I just can't believe it was wanted to behave like this.
      Since I don't understand the class (and didn't manage to extract the intended behaviour in the first place), I'm sorry I don't have a fix or suggestion.

      At least I checked the sources in tag "magnolia-module-blossom-3.0-alpha1" before filing this bug. Class info.magnolia.module.blossom.support.SpecialAttributeRequestWrapper doesn't seem to be modified at all from the version I am using, so the problem should still be there.

      Now, back to the beginning. The bug with the nested areas referring the same JSPs where the page request suddenly fails with a weird exception originates from line 104 in SpecialAttributeRequestWrapper.java where it says:

      if (localValues[i] != null && !ObjectUtils.equals(super.getAttribute(specialNames[i]), originalValues[i])) {
      

      This line of code is executed dozens or hundreds of times per request and compares many things from the different processing stages in all conceivable combinations, so at one point it also compares the name of the JSP file name attached to one area with the JSP file name attached to the other area.
      If there is a difference, shouldMasquarade() returns false, so there is no masquarading and the correct JSP path is ultimately used and the page is rendered correctly.
      If however there is no difference, shouldMasquarade() returns true, so the correct JSP path which would make the page to be rendered correctly is masked by a value that doesn't work ("/startPageSlider" in my case, as said in the exception mentioned earlier).
      I'm 99% sure that this behaviour is a side effect of the problems mentioned earlier with this class and is not based on any logical or functional requirement as such.

      I hope my effort is helping you to develop Blossom further. Thank you.

        Acceptance criteria

              tmattsson Tobias Mattsson
              tln TLN
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Created:
                Updated:
                Resolved:

                  Bug DoR
                  Task DoD