-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
tpl/partials: Make sure a cached partial is invoked only once #9508
Conversation
OK, I didn't get the new numbers from the "template hints" output to add up, so I adjusted the logic a little and added a test case. @davidsneighbour @moorereason @jmooring some feedback on this would be appreciated, ref. my test output above: cumulative average maximum cache percent cached total
duration duration duration potential cached count count template
---------- -------- -------- --------- ------- ------ ----- --------
163.334µs 163.334µs 163.334µs 0 0 0 1 index.html
23.749µs 5.937µs 19.916µs 25 50 2 4 partials/dynamic1.html
9.625µs 4.812µs 6.75µs 100 50 1 2 partials/static1.html
7.625µs 7.625µs 7.625µs 100 0 0 1 partials/static2.html My statement about A cached template gets executed once before it gets cached, so the "percent cached" will never be 100 could be questioned ... An example of that would be |
93c524b
to
1ba7573
Compare
I agree with your comments @bep and the revisions (at least from looking at it on my phone). Would it be clearer if the "cache count" column was called "cache hits"? |
Test failing on Windows, happy times... |
a434add
to
4942c93
Compare
@moorereason we can rename the column(s) later. |
4942c93
to
52ac501
Compare
This commit revises the locking strategy for `partialCached`. We have added a benchmark that may be a little artificial, but it should at least show that we're not losing any performance over this: ```bash name old time/op new time/op delta IncludeCached-10 12.2ms ± 2% 11.3ms ± 1% -7.36% (p=0.029 n=4+4) name old alloc/op new alloc/op delta IncludeCached-10 7.17MB ± 0% 5.09MB ± 0% -29.00% (p=0.029 n=4+4) name old allocs/op new allocs/op delta IncludeCached-10 128k ± 1% 70k ± 0% -45.42% (p=0.029 n=4+4) ``` This commit also revises the template metrics hints logic a little, and add a test for it, which output is currently this: ```bash cumulative average maximum cache percent cached total duration duration duration potential cached count count template ---------- -------- -------- --------- ------- ------ ----- -------- 163.334µs 163.334µs 163.334µs 0 0 0 1 index.html 23.749µs 5.937µs 19.916µs 25 50 2 4 partials/dynamic1.html 9.625µs 4.812µs 6.75µs 100 50 1 2 partials/static1.html 7.625µs 7.625µs 7.625µs 100 0 0 1 partials/static2.html ``` Some notes: * The duration now includes the cached invocations (which should be very short) * A cached template gets executed once before it gets cached, so the "percent cached" will never be 100. Fixes gohugoio#4086 Fixes gohugoio#9506
52ac501
to
fdba8c0
Compare
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This commit revises the locking strategy for
partialCached
. We have added a benchmark that may be a little artificial, but it should at least show that we're not losing any performance over this:This commit also revises the template metrics hints logic a little, and add a test for it, which output is currently this:
Some notes:
Fixes #4086
Fixes #9506