[Bug 64872] New: Inefficient enum resolution in JSPs

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[Bug 64872] New: Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

            Bug ID: 64872
           Summary: Inefficient enum resolution in JSPs
           Product: Tomcat 10
           Version: unspecified
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Jasper
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ------

Our large public-facing application uses many custom JSP tags with enum
properties, for example:

<my:tag enumProperty="${'hotFudge'}"/>

The tag implements MyTag contains a corresponding property:

 public void setEnumProperty(MyEnum enumProperty)

and the enum looks like:

public enum MyEnum {
  hotFudge, whippedCream, cherries;
}

The compiled JSP performs the full resolution of 'hotFudge' -> MyEnum.hotFudge
on *every* request, even though the JSP compiler has enough information to
directly reference the enum.  This results in a significant amount of needless
work; profiling suggests it is around 10% of our JSP processing time.

Note that the same optimization applies when the datatype is a hard-coded
String (like above) or the datatype is boolean/Boolean.

Replacing these evaluations with direct references results in:
1. Lower cpu from not re-processing
2. Lower heap usage because the expression caches are smaller
3. Lower object creation rates because fewer helper objects are created
4. Possible error catching during JSP compilation: if someone refers to a
non-existent enum, that can be caught immediately.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

Mark Thomas <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #1 from Mark Thomas <[hidden email]> ---
Moving this to an enhancement.

Section JSP.2.9 of the JSP specification sets out how references such as
"hotFudge" are to be resolved. Enums are resolved at step 4 (static fields).
For the optimisation to be valid, the compiler needs to be sure that "hotFudge"
will not be resolved in steps 1, 2 or 3.

Step 1 is simple to eliminate. It is the ImplicitObjectELResolver as long as
the name does not match one of the implicit objects then the compiler knows in
advance that this step won't resolve the reference.

Step 2 is potentially problematic. For on-request compilation the compiler can
check whether any resolvers have been added and - possibly - whether one of
those resolvers would resolve the reference or not. Pre-compilation is more
difficult as the compiler cannot know in advance if a resolver is going to be
added via JspApplicationContext.addELResolver(ELResolver) as resolvers may be
added by components of which the JSP compiler is not aware.

Step 3 is simple to eliminate as, for a single attribute, the Stream EL
resolver will not be used.

What is the proposed solution for step 2?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #2 from John Engebretson <[hidden email]> ---
I think you're right that the letter of step #2 proscribes this change.  My
subsequent text in no way attempts to undermine or change that statement.

In the vast majority of cases, when a JSP author uses the value "${true}" and
the property type is Boolean, the author intends Boolean.TRUE.  A case that
violates that would be closer to a bug than desired behavior.

In the vast majority of cases, when a JSP author refers to an enum by implicit
name using the value "${'hotFudge'}" and such an enum exists, the author
intends to use that.  I can conceive of a situation when centralized code wants
to override all values of 'hotFudge' but that is rare at best, and alternative
solutions exist.

In the vast majority of cases, when a JSP author uses a constant string
"${'constant'}" and the targeted property is a String, they intend the property
to contain the string "constant".  Here again I can conceive of a desire to
override all strings with the value "constant" but this would also be rare, and
alternative solutions exist.

Most applications would choose significant efficiency improvements over the
loss of these specific capabilities.

Which leaves us in the situation of a very clear specification versus a very
clear performance win.  How can we resolve this?  My rough ideas:

1. Provide a configuration flag, in which one value enables these optimizations
and one value maintains strict compliance with the spec.  (default value is a
separate question)
2. Fudge the spec, with the argument that the efficiency gains outweigh a few
edge cases.

Thoughts?  Thank you!

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #3 from Mark Thomas <[hidden email]> ---
Fudging the spec is not an option. Providing an option that provides non-spec
compliant behaviour and is disabled by default is an option.

That said, I do wonder why expression language is being used in these instances
rather than a scriptlet.

<my:tag enumProperty="<%= hotFudge %>"/>

rather than

<my:tag enumProperty="${'hotFudge'}"/>

should give you exactly the behaviour you desire shouldn't it?

I'd also expect it be possible to implement with some form of global search and
replace. That approach strikes me as a rather less risky solution (both from an
implementation point of view and a testing one) than an optimisation in the JSP
compiler. It also means you would not be reliant on a Tomcat specific
performance tweak should you decide to switch containers at some point.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #4 from John Engebretson <[hidden email]> ---
"Fudging the spec is not an option." Understood.

"That said, I do wonder why expression language is being used in these
instances rather than a scriptlet."  EL was originally used for this because a)
it hid the details of the enums, b) it worked, c) no one understood the
performance impacts vs. scriptlets, d) scriptlets are discouraged internally
because of maintainability issues.

Your search-and-replace suggestion could work, but given the size of our
codebase and the sheer number of variations, it isn't feasible.

My view of this suggestion is a mechanism for making the existing behavior much
faster - with the unfortunate exception that you pointed out.  Making adoption
free or trivial (config flag) will maximize that benefit for Tomcat users and
would give Tomcat an efficiency advantage over similar containers; in our case
performance matters most and we wouldn't consider switching off Tomcat unless
similar features were available on a competitor.

It sounds like you could support an "accelerated JSP" mode that defaults to
off, and documentation clearly describes how it violates spec?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #5 from Mark Thomas <[hidden email]> ---
Yes, I could support a feature like that.

The tricky part is going to be implementing it. The JSP engine doesn't have
access to the internals of the EL parser. Just musing on that, are all the uses
of the form property="${'literal-string'}" so the process we are trying to
short-cut is the literal to appropriate type coercion?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #6 from John Engebretson <[hidden email]> ---
We have a Tomcat8 implementation deployed that addresses the following cases:

Boolean/boolean properties:
value="true"
value="${true}"
value="${'true'}"

String properties:
value="${'myString'}"

Enum properties:
value="hotFudge"
value="${'hotFudge'}"

We considered the literal conversions to be quite safe, but the others were
debated.
- For booleans we decided that anyone who wrote true but wanted false could fix
their own problem.
- For enums, our codebase contained many places where engineers forgot to use
the literal syntax, so there was substantial value in taking a risk.  We
settled on applying the optimization when the string matched the name of an
enum value, else allow the current EL evaluation to occur as normal.  The
optimized JSPs would clearly reflect the author's intent when valid but
fallback to previous behavior when intent wasn't clear.

We are satisfied with the balance we struck, but I'm interested in your take.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #7 from Mark Thomas <[hidden email]> ---
That all seems reasonable to me.

How big is the patch? Generally, the smaller the patch the fewer the concerns
around size of patch vs how widely used the feature is likely to be.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #8 from John Engebretson <[hidden email]> ---
Our internal patch contained more than one change (the release process was
challenging) but separating out this change comes to around 60 lines of code
across two files.

However, that patch is not up to Tomcat's standards for inclusion (unit tests,
a few design considerations, etc.) so for this purpose I consider it
"inspiration" rather than something we can directly integrate.  And to be
clear, I am personally responsible for those deficiencies.  :)

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #9 from Mark Thomas <[hidden email]> ---
Understood. Any chance you could provide that patch anyway? Saves re-inventing
the wheel.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #10 from John Engebretson <[hidden email]> ---
I'll have to run it past the lawyers, etc.  I'll submit the request
immediately.  Previous submissions have taken between one day and two weeks but
this should be an easy one.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #11 from Mark Thomas <[hidden email]> ---
Tx.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #12 from John Engebretson <[hidden email]> ---
Created attachment 37561
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37561&action=edit
Extract from Tomcat8 optimizations

Approval was rapid, and the extract is attached.  It is not a patch file
because of the issues described previously, however I added comments that I
hope will help.  During this process I noticed what appears to be
double-coverage of the Boolean/boolean case; that could be an error during
development or perhaps I can't remember the good reason for it.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #13 from Mark Thomas <[hidden email]> ---
Thanks for that.

I think it should be possible to use the extension point added for bug 54239 to
implement this. I'll take a look.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #14 from Mark Thomas <[hidden email]> ---
Here is a first pass at a possible approach using ELInterpreter.

This doesn't address the attr="enumValue" case but my reading of the attached
patch is that it doesn't handle that case either. Since that isn't EL a
different approach is required. Enum isn't explicitly listed in JSP.1-11 but it
works because there is a default PropertyEditor for Enums. I suspect bypassing
that would be safe in most cases but there is always the possibility of custom
PropertyEditors so I think a similar hook to ELInterpreter (StringInterpreter?)
would be required.

https://github.com/markt-asf/tomcat/commit/8e7cfc707124b3bb86967d86f9220ed5ab0e5f49

Let me know what you think.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #15 from John Engebretson <[hidden email]> ---
Reading over your commit, it is vastly superior to my implementation, and I am
grateful for your efforts.  :)


The attached patch addresses the attr="enumValue" case using a code branch
towards the bottom of the file, with this comment:

// jengebr - added a branch for enums that are not exact literals, for example
"hotFudge"



Two other points:
1. Good call on the inverted quotes, attr='${"hotFudge"}'.  That never even
crossed my mind.
2. How is this activated?  System property, host context, etc.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #16 from Mark Thomas <[hidden email]> ---
Glad you think it is heading in the right direction. It might not look it but
the important parts are heavily based on the patch you provided.

I am only going from source code rather than testing but I don't think that
enum branch will have an effect. If I am reading the nested if statements
correctly it is in a "if (attr.isELInterpreterInput())" which means "hotFudge"
won't enter that branch because it isn't EL. The "hotFudge" case is handled
later by the PropertyEditor branch.

Activation is via:
- ServletContext attribute (set programmatically)
- ServletContext attribute init parameter (set in web.xml)

Full details at:
https://github.com/markt-asf/tomcat/blob/master/java/org/apache/jasper/compiler/ELInterpreterFactory.java#L50

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #17 from John Engebretson <[hidden email]> ---
Thanks for the activation info, that's perfect.

I've verified the enum case works as I intended on our production servers; the
compiled java code correctly points directly to the enum rather than any
runtime evaluation.  That suggests my code extract missed something relevant.

This particular case is *not* a must-have, but certainly is a nice-to-have.  Is
there a practical way to cover it without overly complicating what you've
already written?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #18 from Mark Thomas <[hidden email]> ---
ACK re the Enum handling. I'll take another look at options for that. I'm still
leaning towards something like a "StringInterpreter".

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64872] Inefficient enum resolution in JSPs

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64872

--- Comment #19 from John Engebretson <[hidden email]> ---
A StringInterpreter is a good idea.  I've had the nagging feeling that there
are other places where this pattern is useful and I just haven't found them
yet.... centralizing the logic will make that easier.

Now that I mention other places, etc. - constant numbers?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12