Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add benchmarks for Sequel with common plugins - #159

Merged
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel
Jan 9, 2023
Merged

Add benchmarks for Sequel with common plugins#159
maximecb merged 7 commits into
ruby:mainfrom
hmistry:sequel

Conversation

@hmistry

Copy link
Copy Markdown
Contributor

Adds benchmarks for Sequel with no plugins and with as many plugins as applicable. Would like to track JIT performance for the Module plugin architecture used in Sequel and Roda where base methods are overridden in plugins. Having both benchmarks allows us to see any impacts due to plugins.

Please advise how you'd like me to setup the 2 different benchmarks - base Sequel and Sequel + plugins. Do you prefer separate folders or 2 files is ok?

@hmistry

Copy link
Copy Markdown
ContributorAuthor

CLA signed. Need help rerunning CI 🙏

@noahgibbs

Copy link
Copy Markdown
Contributor

Currently there's no easy way to return two different measurements from a single benchmark, so "sequel no plugins" and "sequel with plugins" would need to be two different benchmarks.

@noahgibbs

Copy link
Copy Markdown
Contributor

It would be possible to keep your current setup, which gives the "sequel" benchmark, and then add a file called something like "benchmarks/sequel_with_plugins.rb" that changes into the sequel directory before use_gemfile. Basically, make one of them a single-file benchmark while the other is a directory benchmark, but otherwise keep basically the same setup.

@maximecb often has opinions on how many similar benchmarks we want, though, and may or may not be up for two different Sequel benchmarks.

I believe she's away for the holidays, maybe until 5th Jan? So our side may take a bit to reply back here.

With that said, this code looks fine on quick inspection, and I think a Sequel benchmark is a good idea in general.

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@noahgibbs Thanks for the quick review. I made the recommended changes. If @maximecb feels there's no value to having 2 different sequel benchmarks, it's fine with me - happy to change as required. My main goal is to ensure the Module plugin architecture also benefits from YJIT optimizations.

@maximecb

Copy link
Copy Markdown
Contributor

Hi there. Thanks for putting this together @hmistry. Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

I think I would prefer to have just one benchmark for this package. What do you think is the most realistic/typical use case for this gem? Do people usually have many plugins?

Comment threadbenchmarks/sequel/benchmark.rb Outdated
Comment threadbenchmarks/sequel/benchmark.rb
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

@noahgibbs

Copy link
Copy Markdown
Contributor

Database access is something we don't really have in our benchmarks and so it seems like a good idea to benchmark it.

Sequel is probably best thought of as an ActiveRecord competitor, which is presumably why he followed the structure of that benchmark. There are significant differences, but overall it's used for very similar things.

@maximecb

Copy link
Copy Markdown
Contributor

Hi @maximecb. Ok, I'll make this a single benchmark after knowing whether you prefer to be consistent with ActiveRecord benchmark or have a realistic case.

Regarding plugins, yes, a typical application will use several plugins but not all. I'm not sure if you're familiar with this plugin architecture but it's a very minimal vanilla core and then all the functionality is composed using plugins that override the core methods. That way you have the best of both worlds - performance and just the right functionality with nothing extra.

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Comment threadbenchmarks/sequel/benchmark.rb Outdated
@hmistry

Copy link
Copy Markdown
ContributorAuthor

Can you make a judgment call as to which plugins are very commonly used and only include those? 😅

Yes absolutely. I changed to only use a common set of plugins. Hope that'll be enough to determine any YJIT performance optimizations for a chain of method overrides used by the plugin architecture.

Also reduced it to one benchmark per your request. Thank you all for your feedback, please review the changes.

@hmistryhmistry changed the title Add benchmarks for Sequel +/- pluginsAdd benchmarks for Sequel with common pluginsJan 8, 2023
@maximecb

Copy link
Copy Markdown
Contributor

LGTM. Will give @noahgibbs a chance to comment on Monday as well 👍

@noahgibbs

Copy link
Copy Markdown
Contributor

LGTM too!

@maximecb

Copy link
Copy Markdown
Contributor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

@hmistry

Copy link
Copy Markdown
ContributorAuthor

There's just one last detail missing, which is that we should add this benchmark to the list, in the headline category:
https://github.com/Shopify/yjit-bench/blob/main/benchmarks.yml

Done 🙂

@maximecb
maximecb merged commit 7f52737 into ruby:mainJan 9, 2023
@maximecb

Copy link
Copy Markdown
Contributor

Well done @hmistry, and thanks for your patience in getting this merged 🙏 :)

@hmistry

Copy link
Copy Markdown
ContributorAuthor

@maximecb You're welcome! I've been following your work along with MJIT and MIR. I really look forward to seeing YJIT bring significant performance increases to Ruby. Thank you for leading YJIT development! 🙏


run_benchmark(10) do
1.upto(1000) do |i|
post = Post.where(id: i).first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this is actually an inefficient way to query a record in Sequel, Post[i] or Post.first(id: i) are much faster: #207

Sign up for freeto 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.

4 participants

@hmistry@noahgibbs@maximecb@eregon