Modify

Opened 7 years ago

Closed 2 years ago

#7617 closed defect (fixed)

datefield.js is not under the chrome directory

Reported by: Andrew Bates Owned by: Ryan J Ollos
Priority: normal Component: DateFieldPlugin
Severity: normal Keywords: JavaScript htdocs location virtual
Cc: Steffen Hoffmann Trac Release: 0.12

Description (last modified by Ryan J Ollos)

This makes it more difficult to classify datefield.js as a cacheable static resource.

The url is currently: /trac_env/datefield/datefield.js The optimal would be: /trac_env/chrome/datefield/datefield.js

Attachments (6)

DateFieldNotUnderChrome.png (16.0 KB) - added by Ryan J Ollos 5 years ago.
DateFieldPluginNotCachingResource.png (48.6 KB) - added by Ryan J Ollos 5 years ago.
th7617-Trac0.12.patch (5.8 KB) - added by Ryan J Ollos 5 years ago.
ExplicitRefresh.png (39.7 KB) - added by anonymous 5 years ago.
FromCache.png (19.3 KB) - added by anonymous 5 years ago.
OccasionallyOnNavigation.png (43.1 KB) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by Ryan J Ollos

I'm not sure that DateFieldPlugin has control over this. Isn't this under the control of Trac? DateFieldPlugin just calls add_script. Do you have a suggested fix?

comment:2 Changed 5 years ago by Martin

With Apache, I believe this works:

# matches trac static files
AliasMatch ^/(.+)/chrome/common/(.*) /usr/share/pyshared/trac/htdocs/$2
# matches trac plugins static files, e.g. datefields
AliasMatch ^/(.+)/chrome/([^/]*)/(.*) /usr/share/pyshared/$2/htdocs/$3

comment:3 Changed 5 years ago by Ryan J Ollos

Cc: Steffen Hoffmann added; anonymous removed
Description: modified (diff)
Resolution: invalid
Status: newclosed

I don't think this is a plugin issue, but rather an installation issue. Please reopen if this is not the case.

comment:4 Changed 5 years ago by Ryan J Ollos

Resolution: invalid
Status: closedreopened

Changed 5 years ago by Ryan J Ollos

Attachment: DateFieldNotUnderChrome.png added

comment:5 Changed 5 years ago by Ryan J Ollos

Okay, yes I see what you are talking about now:

comment:6 Changed 5 years ago by Ryan J Ollos

From trac.web.chrome:

def add_script(req, filename, mimetype='text/javascript'):
    """Add a reference to an external javascript file to the template.
    
    If the filename is absolute (i.e. starts with a slash), the generated link
    will be based off the application root path. If it is relative, the link
    will be based off the `/chrome/` path.
    """

From post_process_request in datefield.filter:

add_script(req, 'datefield/js/jquery-ui-1.8.16.custom.min.js')
# virtual script
add_script(req, '/datefield/datefield.js')
add_stylesheet(req, 'datefield/css/jquery-ui-1.8.16.custom.css')

Is there any reason that datefield starts with a slash? Does this need to be treated special because it is a virtual script. I'm not sure what that means.

Changed 5 years ago by Ryan J Ollos

comment:7 in reply to:  6 Changed 5 years ago by Ryan J Ollos

Replying to rjollos:

Is there any reason that datefield starts with a slash? Does this need to be treated special because it is a virtual script. I'm not sure what that means.

My interpretation is that it is considered a virtual because since the javascript is contained in the datefield.html template. I see the same pattern in the code for EstimationToolsPlugin. Based on reading code and experimenting, I'm also assuming there is a reason that a virtual script can't be located under /chrome.

My motivation for investigating this issue is that the datefield.js virtual script seems to be one of the big contributors to my page load times.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:8 Changed 5 years ago by Steffen Hoffmann

Had a hard time too before I understood, what's going on there, and especially why.

As I see it, this "virtual script" approach has been chosen to bring in all the variables from Python to JavaScript. But since Trac-0.12dev-r8884 we have add_script_data, and we should possibly backport this to 0.11 to get out of this rather insane situation.

Interesting profiling results. This underlines how much more preferable it is to add a JS file than to add a XHTML template. JS is seemingly light-fast, while (X)HTML is not. I may guess wrong, but don't you think too, that this is most likely due to Genshi's template processing? I bet, changing this to a JS file and adding data with add_script_data will save you roughly a second.

comment:9 in reply to:  8 ; Changed 5 years ago by Ryan J Ollos

Replyng to hasienda:

.. and we should possibly backport this to 0.11 to get out of this rather insane situation.

Are you suggesting that the Trac team needs to backport add_script_data and create Trac 0.11.8? Or is there something we could do ourselves? I read a comment by cboos in which he said that barring a major security issue, there will be no more releases on the 0.11.x line.

JS is seemingly light-fast, while (X)HTML is not. I may guess wrong, but don't you think too, that this is most likely due to Genshi's template processing? I bet, changing this to a JS file and adding data with add_script_data will save you roughly a second.

I don't know enough to even venture a guess, but I will definitely work up a patch today and retest to try to prove your theory. Thank you for your help on this!

If this works, there is potential for speedup on my site by making similar changes to the QuietPlugin.

comment:10 in reply to:  9 ; Changed 5 years ago by Steffen Hoffmann

Replying to rjollos:

Replyng to hasienda:

.. and we should possibly backport this to 0.11 to get out of this rather insane situation.

Are you suggesting that the Trac team needs to backport add_script_data and create Trac 0.11.8? Or is there something we could do ourselves? I read a comment by cboos in which he said that barring a major security issue, there will be no more releases on the 0.11.x line.

Oh, sorry. My wording was misleading here. I know that there's no more work on 0.11, but it's up to us to provide enhanced methods that are not available to older Trac version inside a plugin.

JS is seemingly light-fast, while (X)HTML is not. I may guess wrong, but don't you think too, that this is most likely due to Genshi's template processing? I bet, changing this to a JS file and adding data with add_script_data will save you roughly a second.

I don't know enough to even venture a guess, but I will definitely work up a patch today and retest to try to prove your theory. Thank you for your help on this!

I was lucky to talk to Carsten Klein who did in-deep research in order to improve Genshi in 2009/10. IIRC there's a lot of regenerating objects (deep copy) and recursion in the inner loop of this template engine. And it's rather resource hungry and slow compared to other current alternatives. Still it's versatile and well accepted in the Trac domain, and the discussion about a move towards Jinja2 has never heated again.

So I'm looking forward to your findings.

Changed 5 years ago by Ryan J Ollos

Attachment: th7617-Trac0.12.patch added

comment:11 Changed 5 years ago by Ryan J Ollos

I've attached a patch which seems to work fine in my development environment. I'm going to do some more testing this evening, and then maybe push this to a dev line in the t-h.o repository.

comment:12 in reply to:  10 ; Changed 5 years ago by Ryan J Ollos

Replying to hasienda:

Replying to rjollos:

Replyng to hasienda:

.. and we should possibly backport this to 0.11 to get out of this rather insane situation.

Are you suggesting that the Trac team needs to backport add_script_data and create Trac 0.11.8? Or is there something we could do ourselves? I read a comment by cboos in which he said that barring a major security issue, there will be no more releases on the 0.11.x line.

Oh, sorry. My wording was misleading here. I know that there's no more work on 0.11, but it's up to us to provide enhanced methods that are not available to older Trac version inside a plugin.

Yeah, that is a good idea. In fact, we could make a compatibility class that contains javascript_quote, add_script_data and whatever else we might want to utilize in the future. This script could reside in a single location in the t-h.o repository, and be pulled in to the plugins that need it using an svn:externals property. At a minimum, you and I would probably utilize this in half a dozen plugins. This could be a project on trac-hacks, and the project page could be a place to catalog good practices for making plugin source code work across 0.11 - 0.13dev releases.

comment:13 Changed 5 years ago by Ryan J Ollos

Owner: changed from Robert Corsaro to Ryan J Ollos
Status: reopenednew

Referenced the wrong ticket.

(In [11247]) Refs #9763 (2.0.0dev): Replaced the virtual script (template) datefield.js with a true script. This was possible now that add_script_data is available in Trac 0.12.

Changed 5 years ago by anonymous

Attachment: ExplicitRefresh.png added

Changed 5 years ago by anonymous

Attachment: FromCache.png added

Changed 5 years ago by Ryan J Ollos

comment:14 Changed 5 years ago by Ryan J Ollos

If I navigate around the site, usually I get this result:

Occasionally I get this result:

If I do an explicit page refresh I get:

What I am seeing is that jquery-ui-1.8.16.custom.min.js will be loaded from the cache, except for the case of a page refresh. I'm not sure why datefield.js is usually, but not always loaded from the cache. Any ideas? It's not a big deal though, I'm seeing big improvements from this.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:15 Changed 5 years ago by Ryan J Ollos

Opened #9764, hoping we can do the same thing for the QuietPlugin and maybe I'll get some further improvement in page load times. I'm seeing serious performance issues with permissions checking in the PrivateTicketsPlugin though (4-5 seconds added to /newticket and /ticket), so I'll have to shift my attention to that for the time being.

I'd really appreciate if anyone is able to do testing and give feedback on this.

comment:16 Changed 5 years ago by Ryan J Ollos

Also opened #9765 for possible similar issue with TracTicketTemplatePlugin.

comment:17 in reply to:  12 Changed 5 years ago by Steffen Hoffmann

Keywords: JavaScript htdocs location virtual added

Replying to rjollos:

Replying to hasienda:

Replying to rjollos:

Replyng to hasienda:

.. and we should possibly backport this to 0.11 to get out of this rather insane situation.

Are you suggesting that the Trac team needs to backport add_script_data and create Trac 0.11.8? Or is there something we could do ourselves? I read a comment by cboos in which he said that barring a major security issue, there will be no more releases on the 0.11.x line.

Oh, sorry. My wording was misleading here. I know that there's no more work on 0.11, but it's up to us to provide enhanced methods that are not available to older Trac version inside a plugin.

Yeah, that is a good idea. [...]

This could be a project on trac-hacks, and the project page could be a place to catalog good practices for making plugin source code work across 0.11 - 0.13dev releases.

Pretty much the same as I've been thinking for weeks. A place for good practices, code snippets for common tasks, etc. Glad, you think the same way. Right, I'll push this too, and as soon as there is something visible, announce it to fellow developers on the mailing-list as well.

comment:18 in reply to:  14 ; Changed 5 years ago by Steffen Hoffmann

Replying to rjollos:

What I am seeing is that jquery-ui-1.8.16.custom.min.js will be loaded from the cache, except for the case of a page refresh. I'm not sure why datefield.js is usually, but not always loaded from the cache. Any ideas? ...

Might be a browser cache optimization effect. The cache size is depending on your settings, but always limited. To maximize effect, a cache cleanup policy could include purging small files faster than large ones, because re-getting them wouldn't hurt that much anyway. datefield.js is quite small, in fact one of the smallest pieces in this puzzle, right?

But this is just an "informed guess" from what I've read about HTTP caching strategies.

comment:19 in reply to:  18 Changed 5 years ago by Ryan J Ollos

Replying to hasienda:

... because re-getting them wouldn't hurt that much anyway. datefield.js is quite small, in fact one of the smallest pieces in this puzzle, right?

Yeah, pretty small. When I do more profiling I'll pay attention whether the same behavior exists for other small JS files.

comment:20 Changed 3 years ago by Ryan J Ollos

Status: newassigned

comment:21 Changed 2 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.