Skip to content

Implement Liquid::ResourceLimits#64

Merged
peterzhu2118 merged 4 commits into
masterfrom
pz-resource-limits
Oct 2, 2020
Merged

Implement Liquid::ResourceLimits#64
peterzhu2118 merged 4 commits into
masterfrom
pz-resource-limits

Conversation

@peterzhu2118
Copy link
Copy Markdown
Contributor

Implements Liquid::ResourceLimits in C. It is roughly a translation of the Ruby code into C. I was not able to measure a significant speedup on the benchmarks.

Copy link
Copy Markdown
Contributor

@dylanahsmith dylanahsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the performance improvement would mostly be significant within a capture tag at the moment. So you could try temporarily modifying the benchmark to wrap the benchmarked templates in a capture tag.

I expect it will also help with the assign tag and to reduce the instance variable lookup overhead on the resource limits object.

Comment thread ext/liquid_c/resource_limits.h Outdated
Comment thread ext/liquid_c/resource_limits.c Outdated
Comment thread lib/liquid/c.rb Outdated
Comment thread ext/liquid_c/resource_limits.c Outdated
Comment thread ext/liquid_c/resource_limits.c Outdated
resource_limits_t *resource_limits;

VALUE obj = TypedData_Make_Struct(klass, resource_limits_t, &resource_limits_data_type, resource_limits);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do any initialization here that doesn't depend on the initialize arguments and doesn't need to allocate any objects. In this case, we could even leverage the reset function for that purpose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should move resource_limits_reset(resource_limits); from the initialize method to this allocate function, which will also remove the need for having to use ResourceLimits_Get_Struct(self, resource_limits); there.

Comment thread ext/liquid_c/resource_limits.c Outdated
Comment thread ext/liquid_c/resource_limits.c Outdated
Comment thread ext/liquid_c/vm.c Outdated
Comment thread ext/liquid_c/vm.c Outdated
@peterzhu2118
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @dylanahsmith! I changed the benchmark files to use capture tags, I think there is a slight improvement in performance, however the results are pretty noisy on my computer.

Master:

              parse:    129.494  (±10.0%) i/s -      1.287k in  10.046882s
             render:    135.550  (± 8.9%) i/s -      1.356k in  10.093187s
     parse & render:     62.487  (± 6.4%) i/s -    624.000  in  10.033199s

This branch:

              parse:    131.418  (± 6.8%) i/s -      1.313k in  10.041110s
             render:    149.923  (± 8.7%) i/s -      1.498k in  10.075368s
     parse & render:     65.270  (± 6.1%) i/s -    654.000  in  10.067960s

@dylanahsmith
Copy link
Copy Markdown
Contributor

Wow, that is a more significant difference in the benchmark than I expected.

Comment thread ext/liquid_c/resource_limits.c Outdated
resource_limits_t *resource_limits;

VALUE obj = TypedData_Make_Struct(klass, resource_limits_t, &resource_limits_data_type, resource_limits);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should move resource_limits_reset(resource_limits); from the initialize method to this allocate function, which will also remove the need for having to use ResourceLimits_Get_Struct(self, resource_limits); there.

Comment thread ext/liquid_c/vm.c Outdated
Comment thread ext/liquid_c/resource_limits.c Outdated
Comment thread ext/liquid_c/vm.h Outdated
Copy link
Copy Markdown
Contributor

@dylanahsmith dylanahsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor things, otherwise, LGTM

Comment thread ext/liquid_c/resource_limits.c Outdated

void resource_limits_increment_write_score(resource_limits_t *resource_limits, VALUE output)
{
Check_Type(output, T_STRING);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #64 (comment) I meant that this should be done in resource_limits_increment_write_score_method to avoid doing it redundantly in the VM loop.

Comment thread ext/liquid_c/resource_limits.c Outdated

void init_liquid_resource_limits()
{
id_bytesize = rb_intern("bytesize");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused, so can be removed now.

@peterzhu2118 peterzhu2118 merged commit c984997 into master Oct 2, 2020
@peterzhu2118 peterzhu2118 deleted the pz-resource-limits branch October 2, 2020 17:15
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