Skip to content

Randomized string concat - #216

Merged
etagwerker merged 9 commits into
fastruby:mainfrom
alekseyl:randomized_string_concat
Jun 17, 2023
Merged

etagwerker merged 9 commits into
fastruby:mainfrom
alekseyl:randomized_string_concat

Conversation

@alekseyl

Copy link
Copy Markdown
Contributor

proposition for #158 fix.

@JuanVqz JuanVqz left a comment •

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.

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 👍

Comment thread code/string/concatenation_randomized.rb Outdated
Comment thread code/string/concatenation_randomized.rb Outdated
Comment thread code/string/concatenation_randomized.rb Outdated
Comment thread code/string/concatenation_randomized.rb Outdated
Comment thread code/string/concatenation_randomized.rb Outdated
Aleksey Leshchuk and others added 5 commits January 7, 2023 15:16
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
@alekseyl

alekseyl commented Jan 7, 2023

Copy link
Copy Markdown
Contributor Author

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

Comment thread code/string/concatenation_randomized.rb Outdated

@JuanVqz JuanVqz left a comment

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.

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

@JuanVqz

JuanVqz commented Jan 12, 2023

Copy link
Copy Markdown
Member

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

@etagwerker etagwerker left a comment

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.

@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
Copy Markdown
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

ixti commented Apr 27, 2023

Copy link
Copy Markdown
Collaborator

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}"

@etagwerker etagwerker left a comment

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.

@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