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
Fixes #20776 - Do not re-render template if it can be loaded #371
Conversation
@@ -161,7 +161,7 @@ def invocation_result(invocation, key) | |||
|
|||
def preview_box(template_invocation, target) | |||
renderer = InputTemplateRenderer.new(template_invocation.template, target, template_invocation) | |||
if (preview = renderer.preview) | |||
if preview = (load_template_from_task(template_invocation, target) || renderer.preview) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint/AssignmentInCondition: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
When the user visits the job invocation details page, we rendered the templates for first 20 hosts, which made the page load slowly. With this change we first try to look into a task for this host, which may contain the already rendered template.
57fc848
to
d8a4e5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question, otherwise looks good so far, I'll test it in a bit
@@ -161,7 +161,7 @@ def invocation_result(invocation, key) | |||
|
|||
def preview_box(template_invocation, target) | |||
renderer = InputTemplateRenderer.new(template_invocation.template, target, template_invocation) | |||
if (preview = renderer.preview) | |||
if (preview = (load_template_from_task(template_invocation, target) || renderer.preview)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the conditions for input not yet have rendered template? e.g. future execution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future execution the task object would not exist at all (only the parent task exists at that time). This is to prevent a race condition where the RunHostJob
already exists, but its ProxyAction
was not planned yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I failed to look at your benchmarks, but you convinced me already :-) I'll do a quick test, but 👍 so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I should have attached the benchmarks to the PR
Not probably the best of benchmarks but should give a rough idea about this affects performance def benchmark(count, template_invocation, target)
require 'benchmark'
Benchmark.benchmark do |bm|
bm.report('db load') do
count.times { load_template_from_task(template_invocation, target) }
end
bm.report('render') do
count.times do
renderer = InputTemplateRenderer.new(template_invocation.template, target, template_invocation)
renderer.preview
end
end
end
end
pry> template_invocation.template.template
"<%= input(\"command\") %>"
pry> benchmark 100, template_invocation, target
db load 0.061721 0.000000 0.061721 ( 0.062268)
render 0.310252 0.021698 0.331950 ( 0.329095)
pry> template_invocation.template.name
"Package Action - SSH Default"
pry> benchmark 100, template_invocation, target
db load 0.207657 0.027209 0.234866 ( 0.337088)
render 3.192303 0.043801 3.236104 ( 3.247061) |
Thanks @adamruzicka, works well! Failures are unrelated, merging. |
…man#371) When the user visits the job invocation details page, we rendered the templates for first 20 hosts, which made the page load slowly. With this change we first try to look into a task for this host, which may contain the already rendered template.
When the user visits the job invocation details page, we rendered the
templates for first 20 hosts, which made the page load slowly. With
this change we first try to look into a task for this host, which may
contain the already rendered template.