Skip to content

support builtin for Kernel#clone - #2954

Merged
nobu merged 2 commits into
ruby:masterfrom
S-H-GAMELINKS:support_builtin_kernel_clone
Mar 17, 2020
Merged

support builtin for Kernel#clone#2954
nobu merged 2 commits into
ruby:masterfrom
S-H-GAMELINKS:support_builtin_kernel_clone

Conversation

@S-H-GAMELINKS

@S-H-GAMELINKS S-H-GAMELINKS commented Mar 9, 2020

Copy link
Copy Markdown
Contributor

using builtin for Kernel#clone method.

@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch 2 times, most recently from 96ce2c6 to 8a1d74c Compare March 9, 2020 17:21
@ko1

ko1 commented Mar 9, 2020

Copy link
Copy Markdown
Contributor

could you check performance?

@S-H-GAMELINKS

S-H-GAMELINKS commented Mar 9, 2020

Copy link
Copy Markdown
Contributor Author

Yes, did it.

check benchmark, this code.

require 'benchmark_driver'

Benchmark.driver do |x|
  x.prelude <<~RUBY
    obj = Object.new
  RUBY

  x.report 'origin non freeze', %{ clone = obj.clone }
  x.report 'builtin non freeze', %{ clone = obj.patch_clone }
end

Benchmark.driver do |x|
  x.prelude <<~RUBY
    obj = Object.new
  RUBY

  x.report 'origin freeze', %{ clone = obj.clone(freeze: true) }
  x.report 'builtin freeze', %{ clone = obj.patch_clone(freeze: true) }
end

and, result.

sh@MyComputer:/mnt/c/users/shunh/desktop/rubydev/benchmark$ ../install/bin/ruby benchmark.rb
Warming up --------------------------------------
   origin non freeze     6.152M i/s -      6.218M times in 1.010765s (162.55ns/i, 456clocks/i)
  builtin non freeze     5.111M i/s -      5.125M times in 1.002616s (195.65ns/i, 549clocks/i)
Calculating -------------------------------------
   origin non freeze     6.775M i/s -     18.456M times in 2.723969s (147.59ns/i, 414clocks/i)
  builtin non freeze     5.362M i/s -     15.334M times in 2.859671s (186.50ns/i, 523clocks/i)

Comparison:
   origin non freeze:   6775338.0 i/s
  builtin non freeze:   5362017.6 i/s - 1.26x  slower

Warming up --------------------------------------
       origin freeze     3.331M i/s -      3.392M times in 1.018313s (300.19ns/i, 842clocks/i)
      builtin freeze     5.016M i/s -      5.033M times in 1.003497s (199.37ns/i, 559clocks/i)
Calculating -------------------------------------
       origin freeze     2.999M i/s -      9.994M times in 3.332212s (333.44ns/i, 936clocks/i)
      builtin freeze     5.737M i/s -     15.047M times in 2.622901s (174.31ns/i, 489clocks/i)

Comparison:
      builtin freeze:   5736928.3 i/s
       origin freeze:   2999068.5 i/s - 1.91x  slower

@S-H-GAMELINKS

S-H-GAMELINKS commented Mar 9, 2020

Copy link
Copy Markdown
Contributor Author

But, this patch is not enough. Fail in test code like that.

irb(main):001:0> o = Object.new                                                                                                                                                              irb(main):002:0> o.clone(freeze: false)                                                                                                                                                      Traceback (most recent call last):
        6: from ../install/bin/irb:23:in `<main>'
        5: from ../install/bin/irb:23:in `load'
        4: from /mnt/c/Users/shunh/Desktop/rubydev/install/lib/ruby/gems/2.8.0/gems/irb-1.2.3/exe/irb:11:in `<top (required)>'
        3: from (irb):2
        2: from <internal:kernel>:27:in `clone'
        1: from <internal:kernel>:27:in `initialize_clone'
ArgumentError (wrong number of arguments (given 2, expected 1))
irb(main):003:0>

gived freeze: false caused ArgumentError

nobu
nobu reviewed Mar 10, 2020
View reviewed changes
Comment thread object.c Outdated
{
if (freeze == Qfalse) return FALSE;

if (freeze != Qundef && freeze != Qtrue)

@nobu nobu Mar 10, 2020

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.

freeze can't be Qundef.

nobu
nobu reviewed Mar 10, 2020
View reviewed changes
Comment thread object.c Outdated

if (!special_object_p(obj))
return mutable_obj_clone(obj, kwfreeze);
return mutable_obj_clone(obj, kwfreeze);

@nobu nobu Mar 10, 2020

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.

The indent seems broken.

@nobu

nobu commented Mar 10, 2020

Copy link
Copy Markdown
Member

Please add a benchmark file under benchmark/ when you have it, if the performance is a main concern.

@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch 2 times, most recently from c757b55 to 226fcc6 Compare March 11, 2020 13:20
@S-H-GAMELINKS

S-H-GAMELINKS commented Mar 11, 2020

Copy link
Copy Markdown
Contributor Author

Fix ArgumentError and, added kernel_clone.yml in /benchmark.

@nobu

nobu commented Mar 12, 2020

Copy link
Copy Markdown
Member

mutable_obj_clone should equal obj_mutable_obj_clone since 83e9d6b, as calling RB_PASS_CALLED_KEYWORDS was useless.

@nobu

nobu commented Mar 12, 2020

Copy link
Copy Markdown
Member

You have to apply the difference at https://travis-ci.org/github/ruby/ruby/jobs/661078945#L2071-L2101

@nobu

nobu commented Mar 12, 2020

Copy link
Copy Markdown
Member

kernel.rb has 4-column indents, but the other bundled ruby scripts have 2-column indents.
Also it lacks a newline at EOF.

nobu
nobu reviewed Mar 12, 2020
View reviewed changes
Comment thread object.c
@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch 6 times, most recently from 6673d8d to 523360d Compare March 15, 2020 07:38
@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch from 523360d to f6a54e6 Compare March 15, 2020 07:47
@S-H-GAMELINKS S-H-GAMELINKS reopened this Mar 15, 2020
nobu
nobu reviewed Mar 16, 2020
View reviewed changes
Comment thread benchmark/kernel_clone.yml Outdated
clone: "object.clone"
clone_true: "object.clone(freeze: true)"
clone_false: "object.clone(freeze: false)"
loop_count: 10000 No newline at end of file

@nobu nobu Mar 16, 2020

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.

Missing newline.

@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch 3 times, most recently from ab7861d to 54f1a5d Compare March 16, 2020 02:02
nobu
nobu approved these changes Mar 16, 2020
View reviewed changes
Comment thread benchmark/kernel_clone.yml Outdated
clone_true: "object.clone(freeze: true)"
clone_false: "object.clone(freeze: false)"
loop_count: 10000

No newline at end of file

@nobu nobu Mar 16, 2020

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.

Why an excess space?

@S-H-GAMELINKS S-H-GAMELINKS Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that's typo

Comment thread kernel.rb Outdated
__builtin_rb_obj_clone2(freeze)
end
end

No newline at end of file

@nobu nobu Mar 16, 2020

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.

Also here.

Comment thread object.c Outdated
int kwfreeze = obj_freeze_opt(freeze);
if (!special_object_p(obj))
return mutable_obj_clone(obj, kwfreeze);
return mutable_obj_clone(obj, kwfreeze);

@nobu nobu Mar 16, 2020

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.

Excess spaces at the beginning.

Comment thread object.c Outdated
rb_raise(rb_eArgError, "unexpected value for freeze: %"PRIsVALUE,
rb_obj_class(kwfreeze));
}
ret = obj_freeze_opt(kwfreeze);

@nobu nobu Mar 16, 2020

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.

Currently only freeze option is used and kwfreeze can't be Qundef here, but it would be safer to check for it.
And trailing spaces.

@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch from 54f1a5d to 22a3ba0 Compare March 16, 2020 02:17
@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch 6 times, most recently from fc97a55 to bb9fc3c Compare March 16, 2020 12:00
@S-H-GAMELINKS
S-H-GAMELINKS force-pushed the support_builtin_kernel_clone branch from bb9fc3c to dc189bd Compare March 16, 2020 12:48
@nobu
nobu merged commit 290d608 into ruby:master Mar 17, 2020
@S-H-GAMELINKS
S-H-GAMELINKS deleted the support_builtin_kernel_clone branch March 17, 2020 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nobu nobu nobu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@S-H-GAMELINKS @ko1 @nobu