django-template-partials icon indicating copy to clipboard operation
django-template-partials copied to clipboard

Fix #61: Return correct source for a partial.

Open vsajip opened this issue 10 months ago • 8 comments

I've had a crack at fixing #61. No doubt you'll find something to improve!

vsajip avatar Jan 24 '25 19:01 vsajip

The module constant (and signal use) seems like overkill to me. Why not just find the partial by name when requested, making it a cached property if needed?

If it's a cached property, presumably it wouldn't update if the template changed on disk? Is your concern about memory usage of the partials maps, or something else? What's the issue with using the file changed signal? I certainly find it useful during development to just change the template and have the changes take effect on the next request. In my Slippers use case, the source of the partial needs to be up to date when the component is rendered, including any data computed in the preamble (which might have been changed in a template edit).

vsajip avatar Jan 25 '25 11:01 vsajip

Ok, then same but don't cache it. ("…if needed")

Just calculate it on demand and return it. (I'm not seeing why we need the global map)

carltongibson avatar Jan 25 '25 11:01 carltongibson

OK, removed caching, including the global map.

(I'm not seeing why we need the global map)

For performance on large templates with lots of partials (saves having to scan the origin template lots of times). Use case is component-based UI with lots of small partials.

vsajip avatar Jan 25 '25 11:01 vsajip

OK, it looks more like how I'd imagined.

I'd like to see actual numbers before introducing a performance optimisation.

Have a play with it in your project, and see if it works for you.

carltongibson avatar Jan 25 '25 11:01 carltongibson

Q: How long does compiling the Regex take? Would it be worth trying an on-demand regex that includes the partial's name (for which we'd expect just a single match)?

carltongibson avatar Jan 25 '25 12:01 carltongibson

I changed the finder function to this, looking for a specific partial:

    def find_partial_source(self, full_source, partial_name):
        """
        Search the full source of the template, looking for the sought partial
        and returning it if found, else the empty string.
        """
        result = ''
        start_tag = re.compile(r'\{%\s*(startpartial|partialdef)\s+(' + partial_name + r')(\s+inline)?\s*%}')
        m = start_tag.search(full_source)
        if m:
            sspos, sepos = m.span()
            starter, name, inline = m.groups()
            end_tag = _END_TAG_OLD if starter == 'startpartial' else _END_TAG
            endm = end_tag.search(full_source, sepos + 1)
            assert endm, 'End tag must be present'
            espos, eepos = endm.span()
            result = full_source[sepos:espos]
        return result

and then set up this simple end-to-end test to time the rendering:

import timeit

from django.test import TestCase, Client

class PartialsTestCase(TestCase):
    def setUp(self):
        pass

    def test_render_performance(self):
        n = 500
        t = timeit.timeit(setup="from django.test import Client; c = Client(headers={'user-agent': 'Mozilla/5.0 (X11; Linux x86_64; rv:134.0) Gecko/20100101 Firefox/134.0'})",
                          stmt="c.get('/test')", number=n)
        print(f'{int(t * 1000/n)} msecs')

I found that with caching, the time is handily better than without, even when the non-caching version looks for just the one partial. Results without caching:

$ python manage.py test
Found 1 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
203 msecs
.
----------------------------------------------------------------------
Ran 1 test in 105.595s

OK
Destroying test database for alias 'default'...

Results with caching, looping through all partials:

$ python manage.py test
Found 1 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
56 msecs
.
----------------------------------------------------------------------
Ran 1 test in 28.399s

OK
Destroying test database for alias 'default'...

So, 56 msecs caching vs. 203 msecs non-caching seems like a win for caching to me.

BTW, the timing test is limited to 500 requests because for some reason, the memory usage blows up to use all available memory. I'm not sure if there's some kind of leak in the test client hanging on to requests/responses. The memory still went up with 500 requests, but the test managed to complete :smile:

vsajip avatar Jan 25 '25 13:01 vsajip

What's the test view look like there? (How often in real life is the template source accessed?)

carltongibson avatar Jan 25 '25 14:01 carltongibson

It's an HTMX application, and the test view extends the basic layout to display a specific component or two being tested. The basic layout contains a lot of components in the "chrome" of the application - navigation elements, hidden modals for sign up/sign in which are displayed using JavaScript, and so on. Here's a partial list of components from my Slippers components.yaml:

    clear_icon:     'base/components.html#clear_icon'
    filter_row:     'base/components.html#filter_row'
    form_field:     'base/components.html#form_field'
    default_button: 'base/components.html#default_button'
    link:           'base/components.html#link'
    nav_item:       'base/components.html#nav_item'
    font_icon:      'base/components.html#font_icon'
    modal:          'base/components.html#modal'
    modal_close:    'base/components.html#modal_close'
    modal_header:   'base/components.html#modal_header'
    modal_body:     'base/components.html#modal_body'
    modal_footer:   'base/components.html#modal_footer'
    tabs:           'base/components.html#tabs'
    tab:            'base/components.html#tab'
    captcha:        'base/components.html#captcha'

Of course, the components.html template, and the sources of the individual partials within it, are accessed a lot (by Slippers) when the page is rendered.

vsajip avatar Jan 25 '25 16:01 vsajip