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

Add an instrument option to disable generation of native block bodies #89

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

macournoyer
Copy link
Contributor

Problem

As mentioned in #80 (comment), currently all shops profiled through the Theme Inspector don't see the profiling of the section templates.

Screen Shot 2020-10-13 at 2 32 41 PM

(This should be 6 levels deep)

Solution

We need a way to disable generation of native BlockBody::C when profiling will happen later, but don't want to profile yet.

The new option is to be used here: https://github.com/Shopify/storefront-renderer/blob/master/app/liquid/theme_render_context.rb#L265-L269

The name was to imply parse this for profiling (instrument it), but don't profile it. I imagined in the future, it will mean: generate byte-code that is instrumented. But opened to suggestions on the name.

This is to allow section template to be parsed for profiling upfront.
See https://github.com/Shopify/storefront-renderer/blob/master/app/liquid/theme_render_context.rb#L265-L269

The name was to imply parse this for profiling (instrument it), but don't profile it.
@macournoyer
Copy link
Contributor Author

Setting profile: true for the section template's ParseContext cause a nil exception on line https://github.com/Shopify/liquid/blob/master/lib/liquid/profiler.rb#L90. I tried fixing that, but that didn't solve the issue of section profiling not showing up. So this seems like the simplest solution to buy some time before introducing profiling in the VM.

@dylanahsmith
Copy link
Contributor

This sounds like it might be an application specific issue, since it has to do with sections and not just the include/render tag.

It sounds like the intent of the instrument: true option is the same as the profile: true parse, which is to parse the template for profiling.

I think we could just name the option :disable_liquid_c_nodes which could be useful more generally, such as to use liquid parse tree.

@macournoyer
Copy link
Contributor Author

Done. Will add a TODO in SFR code to remove it once liquid-c gets profiling support.

@macournoyer macournoyer merged commit 30500d4 into master Oct 15, 2020
@macournoyer macournoyer deleted the disable-on-instrument branch October 15, 2020 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants