Skip to content

Improve performance for accessing files from IDOM_WEB_MODULES_DIR #4

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

Closed
2 of 3 tasks
rmorshea opened this issue Aug 17, 2021 · 10 comments
Closed
2 of 3 tasks

Improve performance for accessing files from IDOM_WEB_MODULES_DIR #4

rmorshea opened this issue Aug 17, 2021 · 10 comments

Comments

@rmorshea
Copy link
Contributor

rmorshea commented Aug 17, 2021

IDOM_WEB_MODULES_DIR contains JS modules that are dynamically added at runtime. Since JS files are typically loaded statically, it would be nice if we could improve how quickly these files can be accessed. There are three ways to do that:

  • Implement a management command for collecting these static files ahead of time.
  • Make the view accessing the files async to avoid blocking while waiting on the file system
  • Cache this view using Django's cache framework.

Notes

To implement the management command we could trace over all the templates and look for idom_component template tag nodes in order to figure out what modules to import so that we can trigger the collection of the JS files into IDOM_WEB_MODULES_DIR. Then once they've been collected those files can be copied into a permanent static location.

To do this we can take inspiration from the django compressor library which takes a similar approach. See the COMPRESS_OFFLINE setting to figure out how they're doing it. Also the compressor.offiline module contains uniform interfaces for parsing template syntax for Django and Jinja that we might want to copy (with the latest LICENSE, what to do about AUTHORS file?).


Conversation originated from: this comment

@rmorshea
Copy link
Contributor Author

@Archmonger I'm looking at what's required to implement this and its kind of ugly stuff. I'd mostly be copy-pasting code I don't really understand which makes me nervous about maintaining it going forward. I'm also still a bit skeptical that this will actually be a bottle neck. Given that, I think I'm going to hold off for now. If it does prove to be a real issue, we can revisit at that point.

With that said, I think there are two simpler solutions to this problem of the dynamic files in IDOM_WEB_MODULES_DIR that can improve performance and will be easier to maintain:

  1. Make the view reading these web modules async so it can await the file instead of blocking until the file system is available.
  2. Allow users to configure a cross-process cache using Django's cache framework.

@rmorshea rmorshea changed the title Add static web module collection method Improve performance for accessing files from IDOM_WEB_MODULES_DIR Aug 18, 2021
@Archmonger
Copy link
Contributor

Archmonger commented Aug 18, 2021

Async

I agree that it should be async. Generally all functions and views that target Django 3.0+ should be async. That is, unless you're writing Django code that uses calls the DB (Django team is still working on async DB ORM).


HTML tag parsing

I also was having trouble following the django-compressor code, but I'll see if I can digest it to become maintainable.

If not, I do have another idea in mind to accomplish the same thing.

We could directly call all configured staticfile finders and, as you suggested before, do a regex match on all .html files for the idom component tag.


Bottleneck?

To my understanding, all JS Web Modules should become apparent on the idom component's first call. If that is indeed the case then I'm not seeing any edge cases of this implementation.

But, if for some reason IDOM does not deterministically collect all required web modules on first call, then we'd need to reasses.


Caching

I do support us caching the view. But, not all Django users run with any cache backends enabled, and additionally not all cache backends are multiprocessing compatible. I recall the only built-in that is multiprocessing compatible is memcached. There's a third party backend that's multiprocessing compatible but it's less frequently used.

So perhaps, use Django's django.views.decorators.cache.cache_page when the user has a cache backend enabled (settings.py:CACHE), and fallback to functools.cache otherwise.

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 19, 2021

Bottleneck?

I'm also still a bit skeptical that this will actually be a bottle neck.

The this in that statement referred to accessing the JS files. That is, that accessing the JS files using the "dynamic" approach (as apposed to being able to load them from a static dir) would become a bottleneck.


For now, I think I'm going to make the function async, and add the basic caching with functools we'd already talked about. I'll leave this issue open, and mark with with the "future" label.

Since a static collection method would be a backwards compatible addition, I think I feel comfortable releasing with the aforementioned changes so I can move on to other things in idom core.

@Archmonger
Copy link
Contributor

Archmonger commented Aug 19, 2021

If we do intend to implement this as a management command later, I've determined the relevant lines of code related to django_compressor. Although their code is severely lacking comments, I personally think it's fairly easy to follow.

See django-compressor/compress.py:80-233.

Django Compressor Psuedocode

  1. Create a list of all templates via get_loaders().
  2. Collect all non-broken templates into a list. Broken syntax is checked via jinja2 parse() or django parse().
  3. Perform _compress_template on each templatetag via a ThreadPoolExecutor.

Future Implementation

Since we're leaving this for later, we're going to need to follow in django-compressor's footsteps of having a COLLECT_OFFLINE setting for backwards compatibility.

@Archmonger
Copy link
Contributor

Archmonger commented Aug 19, 2021

On the note of performance, we may also want to consider caching the output of _register_component. I'm not sure if that'll have any ramifications though.

@rmorshea
Copy link
Contributor Author

This check should cover that since we avoid attempting an import if it's already in the set of imported modules.

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 19, 2021

Ah, but one could use the same component more than once, so we could skip registration entirely if it's already there.

Created an issue: #5

@Archmonger
Copy link
Contributor

Archmonger commented Aug 19, 2021

Caching the register function with functools.cache would provide the same ability to skip re-registration, and would allow us to skip the sys.modules check. Could be slightly more performant than checking IDOM_REGISTERED_COMPONENTS.

What do you think?

@Archmonger
Copy link
Contributor

Archmonger commented Jan 16, 2022

Progress Update

A lot of this issue has been resolved.

List of a few things that have been developed

  • Async web module view
  • Web module caching
  • Component tag parser inspired by django-compressor

Task remaining

  • Management command for collecting static files ahead of time

@Archmonger
Copy link
Contributor

Archmonger commented Jan 31, 2022

I'm going to close this issue based on the title.

The fully async web module fetching we have developed is good enough for now.

Will create a separate issue specific to a new collect management command.

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

2 participants