Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only make one HTTP request for TinyMCE plugins #74

Closed
4 tasks done
chillu opened this issue May 17, 2017 · 9 comments
Closed
4 tasks done

Only make one HTTP request for TinyMCE plugins #74

chillu opened this issue May 17, 2017 · 9 comments

Comments

@chillu
Copy link
Member

chillu commented May 17, 2017

Follow up from the "insert link" work, see #58 (comment)

HTTP requests are a major factor in loading performance, even if they're small file sizes. We need to reduce them as much as possible, and the recent TinyMCE changes have gone in the opposite direction - adding four new ones. Figure out how to allow TinyMCE plugins to be loaded determininistically in our app boot, rather than relying on script inclusion order.

/cc @flamerohr

Tasks:

  • Create PHP implementation of tiny_mc_gzip.php that generates bundle from TinyMCEConfig instance
  • Ensure that output of this file is cached to assets via GeneratedAssetHandler
  • Update TinyMCEConfig to point to URLs of generated assets rather than dynamic PHP handler urls

PRS:

@chillu
Copy link
Member Author

chillu commented Jul 12, 2017

Damian said on Slack:

can you please make a call; Is multiple JS plugin downloads better than a framework request? We pretty much need to build a custom tinymce gzip handler to compress these scripts.
1 call to a /tinymce/ url, or 5 http requests to separate js file
it can be cached, but it'll still run up a framework instance
The solution is a lot more complicated than the apparent benefits are turning out to be

Sorry, I can't really make a call about this without more context. The tasks are explaining one type of solution, but not the tradeoffs, or what the alternatives are.

  • We're already using tinymce_gzip.php via TinyMCEConfig? Do we need a (custom?) "PHP implementation" to replace the file_get_contents() with our asset abstraction system? That's a separate problem that already exists in core, right? Do we need a separate issue for it?
  • Why were we able to do this with gzip.php in 3.x, but no longer in 4.x?
  • Can we generate this via dev/build to avoid the overhead of firing off a framework request?

3.x does 55 network requests in live mode, 4.x does 43. So we're already better off. Given how hard this seems to be, I'm fine with lowering the priority - but I want to have a better understanding what the actual problem is first.

@tractorcow
Copy link
Contributor

can you please make a call; Is multiple JS plugin downloads better than a framework request? We pretty much need to build a custom tinymce gzip handler to compress these scripts.
1 call to a /tinymce/ url, or 5 http requests to separate js file
it can be cached, but it'll still run up a framework instance
The solution is a lot more complicated than the apparent benefits are turning out to be

This suggestion is out of date. I'm now discussing pre-generating a static file which is served up as a direct url.

@tractorcow
Copy link
Contributor

We're already using tinymce_gzip.php via TinyMCEConfig? Do we need a (custom?) "PHP implementation" to replace the file_get_contents() with our asset abstraction system? That's a separate problem that already exists in core, right? Do we need a separate issue for it?

The problem isn't the file access. The issue is tiny_mce_gzip.php doesn't allow custom plugin locations (files outside of admin/thirdparty/tinymce). Also, the problem is that it's a new PHP process overhead that should be pre-cachable.

Why were we able to do this with gzip.php in 3.x, but no longer in 4.x?

We weren't in 3.x.

Can we generate this via dev/build to avoid the overhead of firing off a framework request?

We can, or we can just do it in tinymceconfig in the same way combined files works. We would use the same API, just add another generator based on tiny_mce_gzip.php.

@chillu
Copy link
Member Author

chillu commented Jul 12, 2017

The issue is tiny_mce_gzip.php doesn't allow custom plugin locations (files outside of admin/thirdparty/tinymce). Also, the problem is that it's a new PHP process overhead that should be pre-cachable.

So we could "fork" gzip.php to allow inclusion of custom locations?

Why were we able to do this with gzip.php in 3.x, but no longer in 4.x?

I see, the newly loaded separate files are all custom (we've split up framework/thirdparty/tinymce_ssbuttons/editor_plugin_src.js into sset-admin/client/dist/js/TinyMCE_ssmedia.js, asset-admin/client/dist/js/TinyMCE_ssembed.js and a few others)

I've just started writing up a ticket regarding a shared cache, but then figured out it was probably a bad idea. Here's the ticket contents for reference

The cache is currently written to TEMP_FOLDER, which is specific to a single server. In a multi-server setup, this means that multiple servers will each generate their own cache. Since temp folders are pruned (or server instances discarded), in many deployment workflows (incl. SSP) this will cause longer delays for the first author opening the CMS after a deployment on each of these servers.

We should use the built-in core caches to store this data, which can then be adapted to multi-server installations - e.g. a shared memcache. The downside here is that requests will have to go through the framework to get the core cache configuration. So the first request after deployments on multiple servers is faster, but every subsequent request will be slower due to the additional framework overhead.

Note that stuff like this will be especially important for high latency mobile data connections (either used via tethering or on a mobile device)

@chillu chillu removed this from the Recipe 4.0.0-beta2 milestone Jul 12, 2017
@tractorcow
Copy link
Contributor

So we could "fork" gzip.php to allow inclusion of custom locations?

If we are going to invest any time at all into this, I'd rather do away with requiring a PHP request at all.

My plan was to essentially copy the code into a generator, and modify to use a TinyMCEConfig object as a config source, rather than querystring args.

@dhensby
Copy link
Contributor

dhensby commented Jul 17, 2017

With HTTP/2 being pretty prevalent now I'd suggest that the number of HTTP requests is the wrong metric to focus on when considering CMS performance - can we look at how this actually translates in terms of load speed? Will combining these on the server side actually provide a benefit.

Whilst I'm not saying that combining files is bad and we should just rack up as many requests as we can, perhaps removing 3 requests doesn't warrant a large development effort.


NB: almost 70% of HTTP traffic is now HTTP/2 (https://www.keycdn.com/blog/http2-statistics/)

@flamerohr
Copy link
Contributor

Yes, this could be more work than it's worth to reduce 3+ requests where HTTP/2 could already be solving it.

I think using TinyMCEConfig as a config source is a good idea but it does sound like it could cause maintenance issues down the line as well.

@tractorcow
Copy link
Contributor

tractorcow commented Jul 18, 2017

With HTTP/2 being pretty prevalent now I'd suggest that the number of HTTP requests is the wrong metric to focus on when considering CMS performance - can we look at how this actually translates in terms of load speed? Will combining these on the server side actually provide a benefit.

Number of php requests is my suggested metric :) Anything that hits a PHP process is slowed down by a major factor, and combined generated files can be served through static CDN.

@tractorcow
Copy link
Contributor

@tractorcow tractorcow removed their assignment Jul 31, 2017
tractorcow pushed a commit to open-sausages/silverstripe-framework that referenced this issue Aug 1, 2017
tractorcow pushed a commit to open-sausages/silverstripe-framework that referenced this issue Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants