Adding nonstandard support for c:set and c:remove by jengebr · Pull Request #842 · apache/tomcat

jengebr · 2025-04-04T19:38:19Z

Enables optimized, non-standard behavior for the most common JSP tags, when configured to do so.

Impact: benchmark (included)shows 99% reduction in cpu, 100% reduction in object allocation

Activation: off by default, enabled via an init param to the JSP servlet, such as:

      <init-param>
        <param-name>useNonstandardTagOptimizations</param-name>
        <param-value>c:set,c:remove</param-value>
      </init-param>

Example:
Old code (standard): one method, two parameters, 10+ lines of code, one object allocated (and it's large!).

  private boolean _jspx_meth_c_005fset_005f39(javax.servlet.jsp.tagext.JspTag _jspx_th_c_005fwhen_005f11, javax.servlet.jsp.PageContext _jspx_page_context)
          throws java.lang.Throwable {
    javax.servlet.jsp.PageContext pageContext = _jspx_page_context;
    javax.servlet.jsp.JspWriter out = _jspx_page_context.getOut();
    //  c:set
    org.apache.taglibs.standard.tag.rt.core.SetTag _jspx_th_c_005fset_005f39 = new org.apache.taglibs.standard.tag.rt.core.SetTag();
    _jsp_getInstanceManager().newInstance(_jspx_th_c_005fset_005f39);
    try {
      _jspx_th_c_005fset_005f39.setPageContext(_jspx_page_context);
      _jspx_th_c_005fset_005f39.setParent((javax.servlet.jsp.tagext.Tag) _jspx_th_c_005fwhen_005f11);
      // /WEB-INF/views/jsp/features/buybox/offerDisplayGroupLayout.jsp(230,12) name = var type = java.lang.String reqTime = false required = false fragment = false deferredValue = false expectedTypeName = null deferredMethod = false methodSignature = null
      _jspx_th_c_005fset_005f39.setVar("groupName");
      // /WEB-INF/views/jsp/features/buybox/offerDisplayGroupLayout.jsp(230,12) name = value type = javax.el.ValueExpression reqTime = true required = false fragment = false deferredValue = true expectedTypeName = java.lang.Object deferredMethod = false methodSignature = null
      _jspx_th_c_005fset_005f39.setValue(new org.apache.jasper.el.JspValueExpression("/WEB-INF/views/jsp/features/buybox/offerDisplayGroupLayout.jsp(230,12) '${tabContents.keySet().toArray()[0]}'",_jsp_getExpressionFactory().createValueExpression(_jspx_page_context.getELContext(),"${tabContents.keySet().toArray()[0]}",java.lang.Object.class)).getValue(_jspx_page_context.getELContext()));
      int _jspx_eval_c_005fset_005f39 = _jspx_th_c_005fset_005f39.doStartTag();
      if (_jspx_th_c_005fset_005f39.doEndTag() == javax.servlet.jsp.tagext.Tag.SKIP_PAGE) {
        return true;
      }
    } finally {
      org.apache.jasper.runtime.JspRuntimeLibrary.releaseTag(_jspx_th_c_005fset_005f39, _jsp_getInstanceManager(), false);
    }
    return false;
  }

New code (nonstandard) - one parameter, one line of code, zero objects allocated.

  private void _jspx_meth_c_005fset_005f0(jakarta.servlet.jsp.PageContext _jspx_page_context)
          throws java.lang.Throwable {
    //  c:set
    // /jsp/generator/nonstandard/set-01.jsp(18,0) name = var type = java.lang.String reqTime = false required = false fragment = false deferredValue = false expectedTypeName = null deferredMethod = false methodSignature = null
    // /jsp/generator/nonstandard/set-01.jsp(18,0) name = value type = jakarta.el.ValueExpression reqTime = true required = false fragment = false deferredValue = true expectedTypeName = java.lang.Object deferredMethod = false methodSignature = null
    org.apache.jasper.runtime.JspRuntimeLibrary.nonstandardSetTag(_jspx_page_context, "testVar", new org.apache.jasper.el.JspValueExpression("/jsp/generator/nonstandard/set-01.jsp(18,0) '${'testValue'}'",_jsp_getExpressionFactory().createValueExpression(_jspx_page_context.getELContext(),"${'testValue'}",java.lang.Object.class)).getValue(_jspx_page_context.getELContext()), jakarta.servlet.jsp.PageContext.PAGE_SCOPE);
  }

Caller code is simplified by eliminating the boolean check, from:

          if (_jspx_meth_c_005fset_005f39(_jspx_th_c_005fwhen_005f11, _jspx_page_context))
            return true;

to

        _jspx_meth_c_005fset_005f125(_jspx_page_context);

funkman · 2025-04-10T17:20:46Z

While only targeting c:set/c:remove - Do you also need to check the tag belongs to <uri>http://java.sun.com/jsp/jstl/core</uri> ? I suspect the user wouldn't enable the flag then use a different library with the same tag names, but it seems like a possible weird outcome

jengebr · 2025-04-10T17:34:40Z

While only targeting c:set/c:remove - Do you also need to check the tag belongs to <uri>http://java.sun.com/jsp/jstl/core</uri> ? I suspect the user wouldn't enable the flag then use a different library with the same tag names, but it seems like a possible weird outcome

Great suggestion, thank you. Edge cases are a big concern so please keep them coming!

markt-asf

Thinking long term, I am wondering whether we need to provide the ability for users to customise these optimisations. If we do then we can add something like a GeneratorFactory and users can provide their own generator that overrides the built-in one along the lines of ELInterpreterFactory. For now, I don't think is necessary.

markt-asf · 2025-04-15T16:34:39Z

+
+    public static void nonstandardSetTag(jakarta.servlet.jsp.PageContext pageContext, String var, Object value) {
+        if (value == null) {
+            pageContext.removeAttribute(var);
+        } else {
+            pageContext.setAttribute(var, value);
+        }
+    }
+


This only appears to be used in test code. If true, it should be removed.

The generated JSP code calls this method, why is why it's not called within Tomcat itself.

Can you point out where. I only see calls to the 4-arg method, not the 3-arg method.

Thank you, sorry, I missed the 3-arg vs. 4-arg distinction. You're correct and I'll remove the 3-arg.

I verified this by comparison to the JSTL tag and realized I missed some logic to clean up the VariableMapper. I added that as well.

markt-asf · 2025-04-15T16:37:08Z

+            if (scope == null) {
+                out.printil("pageContext.removeAttribute(\"" + varValue + "\");");
+            } else {
+                String scopeValue = translateScopeToConstant(jspAttributes);
+                out.printil("pageContext.removeAttribute(\"" + varValue + "\", " + scopeValue + ");");
+            }


This takes a different approach to handling null scope compared to set. It isn't wrong but it is inconsistent. Why? If there is a good reason, it should be documented else folks coming later are going to wonder why. If there isn't a good reason, set and remove should be consistent (and I have no preference for either approach).

I modeled the code here after the tag implementations (SetTag and RemoveTag), with the goal of maintaining compatibility... so any mismatch is either an error in how I copied the logic, or how the tags themselves are implemented. What specific differences do you see?

set calls

String scopeValue = translateScopeToConstant(jspAttributes);

whereas remove treats scope == null as a special case

if (scope == null) { out.printil("pageContext.removeAttribute(\"" + varValue + "\");"); } else { String scopeValue = translateScopeToConstant(jspAttributes); out.printil("pageContext.removeAttribute(\"" + varValue + "\", " + scopeValue + ");"); }

I'd probably remove the special case treatment from remove.

Weird as this is, I think it matches the logic from RemoveTag:

// removes the variable (from a specific scope, if specified) public int doEndTag() throws JspException { if (!scopeSpecified) pageContext.removeAttribute(var); else pageContext.removeAttribute(var, scope); return EVAL_PAGE; }

And PageContextImpl .remove(var) removes from every scope, whereas .remove(var, scope) only removes from one:.

@Override public void removeAttribute(final String name) { if (name == null) { throw new NullPointerException(Localizer.getMessage("jsp.error.attribute.null_name")); } removeAttribute(name, PAGE_SCOPE); removeAttribute(name, REQUEST_SCOPE); if (session != null) { try { removeAttribute(name, SESSION_SCOPE); } catch (IllegalStateException ise) { // Session has been invalidated. // Ignore and fall throw to application scope. } } removeAttribute(name, APPLICATION_SCOPE); }

Until this effort, I didn't know that remove w/o scope cleared all scopes.

Understood. I didn't realise there was a difference until I looked at the PageContext Javadoc. It is probably worth a short comment on both set and remove on how no/null scope is handled since it is different.

Great suggestion, will do. I'll make a pass on all of this code and add more descriptive comments wherever I got surprised.

jengebr · 2025-04-15T17:02:17Z

Thinking long term, I am wondering whether we need to provide the ability for users to customise these optimisations. If we do then we can add something like a GeneratorFactory and users can provide their own generator that overrides the built-in one along the lines of ELInterpreterFactory.

Honestly, this is where I started, but I had to make a bazillion fields, methods, and classes public or protected. I figured I'd start here and we can bite that off later.

markt-asf

I'm fine with GeneratorFactory being something we consider later if necessary. Just want to make sure we don't do anything now to make it harder to do that if we need to.

markt-asf · 2025-04-16T09:48:33Z

+
+    public static void nonstandardSetTag(jakarta.servlet.jsp.PageContext pageContext, String var, Object value) {
+        if (value == null) {
+            pageContext.removeAttribute(var);
+        } else {
+            pageContext.setAttribute(var, value);
+        }
+    }
+


Can you point out where. I only see calls to the 4-arg method, not the 3-arg method.

markt-asf · 2025-04-16T09:53:02Z

+            if (scope == null) {
+                out.printil("pageContext.removeAttribute(\"" + varValue + "\");");
+            } else {
+                String scopeValue = translateScopeToConstant(jspAttributes);
+                out.printil("pageContext.removeAttribute(\"" + varValue + "\", " + scopeValue + ");");
+            }


set calls

String scopeValue = translateScopeToConstant(jspAttributes);

whereas remove treats scope == null as a special case

if (scope == null) { out.printil("pageContext.removeAttribute(\"" + varValue + "\");"); } else { String scopeValue = translateScopeToConstant(jspAttributes); out.printil("pageContext.removeAttribute(\"" + varValue + "\", " + scopeValue + ");"); }

I'd probably remove the special case treatment from remove.

jengebr marked this pull request as ready for review April 11, 2025 19:30

markt-asf requested changes Apr 15, 2025

View reviewed changes

markt-asf requested changes Apr 16, 2025

View reviewed changes

markt-asf approved these changes Apr 16, 2025

View reviewed changes

jengebr force-pushed the nonstandard_tags branch 4 times, most recently from 5279b3a to 8957f56 Compare April 16, 2025 19:26

Adding nonstandard support for c:set and c:remove

c288103

jengebr force-pushed the nonstandard_tags branch from 8957f56 to c288103 Compare April 16, 2025 19:32

jengebr merged commit 5ca117b into apache:main Apr 16, 2025

jengebr deleted the nonstandard_tags branch April 18, 2025 12:38

Uh oh!

Conversation

jengebr commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

funkman commented Apr 10, 2025

Uh oh!

jengebr commented Apr 10, 2025

Uh oh!

markt-asf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jengebr commented Apr 15, 2025

Uh oh!

markt-asf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jengebr commented Apr 4, 2025 •

edited

Loading