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

Randomized string concat #216

Merged
merged 9 commits into from
Jun 17, 2023
Merged

Conversation

alekseyl
Copy link
Contributor

proposition for #158 fix.

Copy link
Member

@JuanVqz JuanVqz left a comment

Choose a reason for hiding this comment

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

Hey, @alekseyl first of all sorry for the long delay.
Thank you for the time you spent on this, it looks good
I just left some minor comments, let me know what you think 👍

code/string/concatenation_randomized.rb Outdated Show resolved Hide resolved
code/string/concatenation_randomized.rb Outdated Show resolved Hide resolved
code/string/concatenation_randomized.rb Outdated Show resolved Hide resolved
code/string/concatenation_randomized.rb Outdated Show resolved Hide resolved
code/string/concatenation_randomized.rb Outdated Show resolved Hide resolved
@alekseyl
Copy link
Contributor Author

alekseyl commented Jan 7, 2023

Thnx @JuanVqz, no worries for the delay :). I've commited suggested!

Copy link
Member

@JuanVqz JuanVqz left a comment

Choose a reason for hiding this comment

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

@alekseyl, Thank you for your contribution, it looks good 👍

@JuanVqz
Copy link
Member

JuanVqz commented Jan 12, 2023

@mateusdeap could you please have a look to merge it?

Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

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

@alekseyl @JuanVqz I noticed that this PR uses bmbm and we want all benchmarks to use ips

Could you please tweak this benchmark to use "benchmark/ips"?

It shouldn't affect the results.

Thanks!

@JuanVqz JuanVqz requested a review from etagwerker March 8, 2023 23:51
@alekseyl
Copy link
Contributor Author

@alekseyl @JuanVqz I noticed that this PR uses bmbm and we want all benchmarks to use ips
Could you please tweak this benchmark to use "benchmark/ips"?
It shouldn't affect the results.
Thanks!

@etagwerker Hi Ernesto!

Sorry for the delayed answer. The problem is it will not look as good as we migh expect, because of a randomization )). It will show a much bigger variance (20-30-40% I can't recall properly) over relatively "small" difference.

I might give it a try one more time, but can't promise that it will be soon enough :(. I need an inspiration to dive in to that river again ))).

@ixti
Copy link
Collaborator

ixti commented Apr 27, 2023

Can we also add:

[foo, bar].join

It would also make sense to check if there will be different results with more than 2 strings to concatenate. Also, would be nice to see performance comparison of cases like:

"#{foo}-#{bar}"

Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

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

@alekseyl Looks good, thank you!

@etagwerker etagwerker merged commit c12ede2 into fastruby:main Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants