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

Rhai shortcodes: fixing performance issues #91

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

karthik2804
Copy link
Contributor

Adapting #56 to include Rhai shortcodes support for markdown content. Adds a new page head field enable_shortcodes. It does not introduce a significant performance hit as it only runs the markdown content with the shortcode switch through handlebars before the markdown parser.

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Some docs suggestions and then questions around escaping vs not escaping shortcodes.

Also, I'm not savvy on checking for performance as mentioned in the issue -- can @radu-matei advise/test?

docs/content/shortcodes.md Outdated Show resolved Hide resolved
docs/content/shortcodes.md Outdated Show resolved Hide resolved
docs/content/shortcodes.md Outdated Show resolved Hide resolved
docs/content/shortcodes.md Outdated Show resolved Hide resolved
docs/content/shortcodes.md Outdated Show resolved Hide resolved
docs/content/scripting.md Outdated Show resolved Hide resolved
docs/content/shortcodes.md Show resolved Hide resolved
docs/content/shortcodes.md Outdated Show resolved Hide resolved
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Docs and functionality LGTM!

@vdice vdice requested a review from radu-matei July 29, 2022 19:15
@vdice
Copy link
Member

vdice commented Aug 1, 2022

I ran similar tests as mentioned in #56 (comment) against the Bartholomew docs site and noticed perhaps a slight decrease in performance, but I wouldn't say enough to block this feature. Thoughts @radu-matei ?

# with bartholomew.wasm built from main
$ bombardier -c 1 -n 1000 http://localhost:3000/configuration
Bombarding http://localhost:3000/configuration with 1000 request(s) using 1 connection(s)
 1000 / 1000 [===========================================================================================================================================================================================================================================] 100.00% 262/s 3s
Done!
Statistics        Avg      Stdev        Max
  Reqs/sec       270.62      32.70     304.91
  Latency        3.70ms   575.93us    19.79ms
  HTTP codes:
    1xx - 0, 2xx - 1000, 3xx - 0, 4xx - 0, 5xx - 0
    others - 0
  Throughput:     6.01MB/s

# with bartholomew.wasm built from this branch
$ bombardier -c 1 -n 1000 http://localhost:3000/configuration
Bombarding http://localhost:3000/configuration with 1000 request(s) using 1 connection(s)
 1000 / 1000 [===========================================================================================================================================================================================================================================] 100.00% 226/s 4s
Done!
Statistics        Avg      Stdev        Max
  Reqs/sec       230.01      39.73     593.35
  Latency        4.38ms   684.70us    23.39ms
  HTTP codes:
    1xx - 0, 2xx - 1000, 3xx - 0, 4xx - 0, 5xx - 0
    others - 0
  Throughput:     5.12MB/s

@karthik2804
Copy link
Contributor Author

These results are pretty close to what I have. There is some contribution to the decrease in performance due to this branch having an extra page in shortcodes.md to render.

A catch with this is that the more pages that use shortcodes, the slower the rendering time will be. It is because currently, we render every page on every request. Therefore the number of content pages we have will have an impact on the performance metrics.

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

I have a few comments, but nothing major.

Re: performance, the fact that we need to render the additional shortcode does make sense, so I wouldn't be worried by the additional latency here — particularly because you're not incurring it unless you are actively using them.

See below:

$ bombardier -n 100 -c 1 http://localhost:3000/shortcodes
Bombarding http://localhost:3000/shortcodes with 100 request(s) using 1 connection(s)

Done!
Statistics        Avg      Stdev        Max
  Reqs/sec       193.15      33.53     244.26
  Latency        5.18ms     1.09ms    14.69ms
  HTTP codes:
    1xx - 0, 2xx - 100, 3xx - 0, 4xx - 0, 5xx - 0
    others - 0
  Throughput:     4.96MB/s
$ bombardier -n 100 -c 1 http://localhost:3000/shortcodes
Bombarding http://localhost:3000/shortcodes with 100 request(s) using 1 connection(s)

Done!
Statistics        Avg      Stdev        Max
  Reqs/sec       247.07      48.25     305.95
  Latency        4.07ms     1.06ms    13.19ms
  HTTP codes:
    1xx - 0, 2xx - 100, 3xx - 0, 4xx - 0, 5xx - 0
    others - 0
  Throughput:     6.02MB/s

Happy to do another pass at this once you address the comments.
Thanks!

docs/content/configuration.md Outdated Show resolved Hide resolved
docs/content/scripting.md Outdated Show resolved Hide resolved
src/content.rs Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
@radu-matei
Copy link
Member

The last commit you pushed appears as unverified.
Would you mind keeping just a few useful commits and make sure they have a sign-off and GPG signature?

Thanks!

@karthik2804
Copy link
Contributor Author

I am sorry about the last commit being unverified. There was a mess up on my part switching emails. Would you recommend that I squash the commits in this PR?

@radu-matei
Copy link
Member

Yeah, rebasing and squashing the unnecessary commits makes sense.

Signed-off-by: Karthik Ganeshram <karthik.ganeshram@fermyon.com>
@vdice
Copy link
Member

vdice commented Aug 5, 2022

It looks like this is ready to go! Defer to @radu-matei who had the most recent requested changes.

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

LGTM

@vdice
Copy link
Member

vdice commented Aug 12, 2022

Thank you for this great feature @karthik2804!

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.

3 participants