From 75f8aab247abf58b61b3700c078d61ccf7e168bf Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Thu, 18 Jul 2013 07:54:16 +1000 Subject: First round of trying to improve the performance of attr_reader and attr_writer. From ~24sec to ~15sec --- benchmark/ao-render.rb | 42 +++++++++++++----------------------------- mrblib/class.rb | 6 +++--- src/kernel.c | 45 +++++++++++++++++++++++++++++++++++---------- 3 files changed, 51 insertions(+), 42 deletions(-) diff --git a/benchmark/ao-render.rb b/benchmark/ao-render.rb index 793804a7b..e7406b26b 100644 --- a/benchmark/ao-render.rb +++ b/benchmark/ao-render.rb @@ -6,8 +6,8 @@ # mruby version by Hideki Miura # -IMAGE_WIDTH = 256 -IMAGE_HEIGHT = 256 +IMAGE_WIDTH = 64 +IMAGE_HEIGHT = 64 NSUBSAMPLES = 2 NAO_SAMPLES = 8 @@ -30,19 +30,14 @@ module Rand end class Vec + attr_accessor :x, :y, :z + def initialize(x, y, z) @x = x @y = y @z = z end - def x=(v); @x = v; end - def y=(v); @y = v; end - def z=(v); @z = v; end - def x; @x; end - def y; @y; end - def z; @z; end - def vadd(b) Vec.new(@x + b.x, @y + b.y, @z + b.z) end @@ -80,14 +75,13 @@ end class Sphere + attr_reader :center, :radius + def initialize(center, radius) @center = center @radius = radius end - def center; @center; end - def radius; @radius; end - def intersect(ray, isect) rs = ray.org.vsub(@center) b = rs.vdot(ray.dir) @@ -140,33 +134,23 @@ class Plane end class Ray + attr_accessor :org, :dir + def initialize(org, dir) @org = org @dir = dir end - - def org; @org; end - def org=(v); @org = v; end - def dir; @dir; end - def dir=(v); @dir = v; end end class Isect + attr_accessor :t, :hit, :pl, :n + def initialize @t = 10000000.0 @hit = false @pl = Vec.new(0.0, 0.0, 0.0) @n = Vec.new(0.0, 0.0, 0.0) end - - def t; @t; end - def t=(v); @t = v; end - def hit; @hit; end - def hit=(v); @hit = v; end - def pl; @pl; end - def pl=(v); @pl = v; end - def n; @n; end - def n=(v); @n = v; end end def clamp(f) @@ -298,9 +282,9 @@ class Scene r = rad.x / (nsf * nsf) g = rad.y / (nsf * nsf) b = rad.z / (nsf * nsf) - printf("%c", clamp(r)) - printf("%c", clamp(g)) - printf("%c", clamp(b)) + # printf("%c", clamp(r)) + # printf("%c", clamp(g)) + # printf("%c", clamp(b)) end end end diff --git a/mrblib/class.rb b/mrblib/class.rb index 4f268b6c8..d6ef34f17 100644 --- a/mrblib/class.rb +++ b/mrblib/class.rb @@ -2,15 +2,15 @@ class Module # 15.2.2.4.13 def attr_reader(*names) names.each{|name| - name2 = ('@'+name.to_s).intern + name2 = ('@'+name.to_s) define_method(name){self.instance_variable_get(name2)} } end # 15.2.2.4.14 def attr_writer(*names) names.each{|name| - name2 = ('@'+name.to_s).intern - name = (name.to_s+"=").intern + name2 = ('@'+name.to_s) + name = (name.to_s+"=") define_method(name){|v|self.instance_variable_set(name2,v)} } end diff --git a/src/kernel.c b/src/kernel.c index ae8d24710..068b80f62 100644 --- a/src/kernel.c +++ b/src/kernel.c @@ -548,6 +548,14 @@ obj_is_instance_of(mrb_state *mrb, mrb_value self) return mrb_bool_value(instance_of_p); } +static void +valid_iv_name(mrb_state *mrb, mrb_sym id, const char* s, size_t len) +{ + if (len < 2 || !(s[0] == '@' && s[1] != '@')) { + mrb_name_error(mrb, id, "`%S' is not allowed as an instance variable name", mrb_sym2str(mrb, id)); + } +} + static void check_iv_name(mrb_state *mrb, mrb_sym id) { @@ -555,9 +563,23 @@ check_iv_name(mrb_state *mrb, mrb_sym id) size_t len; s = mrb_sym2name_len(mrb, id, &len); - if (len < 2 || !(s[0] == '@' && s[1] != '@')) { - mrb_name_error(mrb, id, "`%S' is not allowed as an instance variable name", mrb_sym2str(mrb, id)); + valid_iv_name(mrb, id, s, len); +} + +static mrb_sym +get_valid_iv_sym(mrb_state *mrb, mrb_value val) +{ + mrb_sym id; + + if (mrb_string_p(val)) { + id = mrb_intern_cstr(mrb, RSTRING_PTR(val)); + valid_iv_name(mrb, id, RSTRING_PTR(val), RSTRING_LEN(val)); + } else if(mrb_symbol_p(val)) { + id = mrb_symbol(val); + check_iv_name(mrb, id); } + + return id; } /* 15.3.1.3.20 */ @@ -582,10 +604,11 @@ mrb_value mrb_obj_ivar_defined(mrb_state *mrb, mrb_value self) { mrb_sym mid; + mrb_value sym; mrb_bool defined_p; - mrb_get_args(mrb, "n", &mid); - check_iv_name(mrb, mid); + mrb_get_args(mrb, "o", &sym); + mid = get_valid_iv_sym(mrb, sym); defined_p = mrb_obj_iv_defined(mrb, mrb_obj_ptr(self), mid); return mrb_bool_value(defined_p); @@ -615,10 +638,11 @@ mrb_value mrb_obj_ivar_get(mrb_state *mrb, mrb_value self) { mrb_sym id; + mrb_value sym; + + mrb_get_args(mrb, "o", &sym); - mrb_get_args(mrb, "n", &id); - - check_iv_name(mrb, id); + id = get_valid_iv_sym(mrb, sym); return mrb_iv_get(mrb, self, id); } @@ -646,10 +670,11 @@ mrb_value mrb_obj_ivar_set(mrb_state *mrb, mrb_value self) { mrb_sym id; - mrb_value val; + mrb_value sym, val; - mrb_get_args(mrb, "no", &id, &val); - check_iv_name(mrb, id); + mrb_get_args(mrb, "oo", &sym, &val); + + id = get_valid_iv_sym(mrb, sym); mrb_iv_set(mrb, self, id, val); return val; } -- cgit v1.2.3 From fe5324bd17d1716592b244762173e7478ebf7853 Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Fri, 19 Jul 2013 22:39:54 +1000 Subject: Wrote tests around attr, attr_reader, attr_writer, attr_accessor --- mrblib/class.rb | 20 ++++--- test/t/module.rb | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 7 deletions(-) diff --git a/mrblib/class.rb b/mrblib/class.rb index 4f268b6c8..0678d943d 100644 --- a/mrblib/class.rb +++ b/mrblib/class.rb @@ -1,18 +1,24 @@ class Module # 15.2.2.4.13 def attr_reader(*names) - names.each{|name| - name2 = ('@'+name.to_s).intern + names.each do |name| + name = name.to_s + raise(NameError, "#{name.inspect} is not allowed as an instance variable name") if name.include?('@') || name.include?('?') || name.include?('$') + + name2 = ('@'+name).intern define_method(name){self.instance_variable_get(name2)} - } + end end # 15.2.2.4.14 def attr_writer(*names) - names.each{|name| - name2 = ('@'+name.to_s).intern - name = (name.to_s+"=").intern + names.each do |name| + name = name.to_s + raise(NameError, "#{name.inspect} is not allowed as an instance variable name") if name.include?('@') || name.include?('?') || name.include?('$') + + name2 = ('@'+name).intern + name = (name+"=").intern define_method(name){|v|self.instance_variable_set(name2,v)} - } + end end # 15.2.2.4.12 def attr_accessor(*names) diff --git a/test/t/module.rb b/test/t/module.rb index 9d735f5da..a03511b09 100644 --- a/test/t/module.rb +++ b/test/t/module.rb @@ -37,6 +37,186 @@ assert('Module#append_features', '15.2.2.4.10') do assert_equal Test4AppendFeatures2.const_get(:Const4AppendFeatures2), Test4AppendFeatures2 end +assert('Module#attr', '15.2.2.4.11') do + %w[ + foo? + @foo + @@foo + $foo + ].each do |name| + assert_raise(NameError) do + module NameTest; end + NameTest.module_eval { attr_reader name.to_sym } + end + end + + class AttrTest + class << self + attr :cattr + def cattr_val + @cattr + end + def cattr_val=(val) + @cattr = val + end + end + attr :iattr + + def iattr_val + @iattr + end + def iattr_val=(val) + @iattr = val + end + end + + test = AttrTest.new + assert_true AttrTest.respond_to?(:cattr) + assert_true test.respond_to?(:iattr) + + assert_false AttrTest.respond_to?(:vattr=) + assert_false test.respond_to?(:iattr=) + + test.iattr_val = 'test' + assert_equal 'test', test.iattr + + AttrTest.cattr_val = 'test' + assert_equal 'test', AttrTest.cattr +end + +assert('Module#attr_accessor', '15.2.2.4.12') do + %w[ + foo? + @foo + @@foo + $foo + ].each do |name| + assert_raise(NameError) do + module NameTest; end + NameTest.module_eval { attr_reader name.to_sym } + end + end + + class AttrTestAccessor + class << self + attr_accessor :cattr + end + attr_accessor :iattr, 'iattr2' + end + + attr_instance = AttrTestAccessor.new + assert_true AttrTestAccessor.respond_to?(:cattr=) + assert_true attr_instance.respond_to?(:iattr=) + assert_true attr_instance.respond_to?(:iattr2=) + assert_true AttrTestAccessor.respond_to?(:cattr) + assert_true attr_instance.respond_to?(:iattr) + assert_true attr_instance.respond_to?(:iattr2) + + attr_instance.iattr = 'test' + assert_equal 'test', attr_instance.iattr + + AttrTestAccessor.cattr = 'test' + assert_equal 'test', AttrTestAccessor.cattr +end + +assert('Module#attr_reader', '15.2.2.4.13') do + %w[ + foo? + @foo + @@foo + $foo + ].each do |name| + assert_raise(NameError) do + module NameTest; end + NameTest.module_eval { attr_reader name.to_sym } + end + end + + class AttrTestReader + class << self + attr_reader :cattr + def cattr_val + @cattr + end + def cattr_val=(val) + @cattr = val + end + end + attr_reader :iattr, 'iattr2' + + def iattr_val + @iattr + end + def iattr_val=(val) + @iattr = val + end + end + + attr_instance = AttrTestReader.new + assert_true AttrTestReader.respond_to?(:cattr) + assert_true attr_instance.respond_to?(:iattr) + assert_true attr_instance.respond_to?(:iattr2) + + assert_false AttrTestReader.respond_to?(:cattr=) + assert_false attr_instance.respond_to?(:iattr=) + assert_false attr_instance.respond_to?(:iattr2=) + + attr_instance.iattr_val = 'test' + assert_equal 'test', attr_instance.iattr + + AttrTestReader.cattr_val = 'test' + assert_equal 'test', AttrTestReader.cattr +end + +assert('Module#attr_writer', '15.2.2.4.14') do + %w[ + foo? + @foo + @@foo + $foo + ].each do |name| + assert_raise(NameError) do + module NameTest; end + NameTest.module_eval { attr_reader name.to_sym } + end + end + + class AttrTestWriter + class << self + attr_writer :cattr + def cattr_val + @cattr + end + def cattr_val=(val) + @cattr = val + end + end + attr_writer :iattr, 'iattr2' + + def iattr_val + @iattr + end + def iattr_val=(val) + @iattr = val + end + end + + attr_instance = AttrTestWriter.new + assert_true AttrTestWriter.respond_to?(:cattr=) + assert_true attr_instance.respond_to?(:iattr=) + assert_true attr_instance.respond_to?(:iattr2=) + + assert_false AttrTestWriter.respond_to?(:cattr) + assert_false attr_instance.respond_to?(:iattr) + assert_false attr_instance.respond_to?(:iattr2) + + attr_instance.iattr = 'test' + assert_equal 'test', attr_instance.iattr_val + + AttrTestWriter.cattr = 'test' + assert_equal 'test', AttrTestWriter.cattr_val +end + assert('Module#class_eval', '15.2.2.4.15') do class Test4ClassEval @a = 11 -- cgit v1.2.3 From 3ebd8167e149c66e7b9a822b196ea610fe07a419 Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Sun, 21 Jul 2013 18:05:45 +1000 Subject: Slight cleanup of code --- src/kernel.c | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/kernel.c b/src/kernel.c index 068b80f62..f2c7f182f 100644 --- a/src/kernel.c +++ b/src/kernel.c @@ -549,37 +549,37 @@ obj_is_instance_of(mrb_state *mrb, mrb_value self) } static void -valid_iv_name(mrb_state *mrb, mrb_sym id, const char* s, size_t len) +valid_iv_name(mrb_state *mrb, mrb_sym iv_name_id, const char* s, size_t len) { if (len < 2 || !(s[0] == '@' && s[1] != '@')) { - mrb_name_error(mrb, id, "`%S' is not allowed as an instance variable name", mrb_sym2str(mrb, id)); + mrb_name_error(mrb, iv_name_id, "`%S' is not allowed as an instance variable name", mrb_sym2str(mrb, iv_name_id)); } } static void -check_iv_name(mrb_state *mrb, mrb_sym id) +check_iv_name(mrb_state *mrb, mrb_sym iv_name_id) { const char *s; size_t len; - s = mrb_sym2name_len(mrb, id, &len); - valid_iv_name(mrb, id, s, len); + s = mrb_sym2name_len(mrb, iv_name_id, &len); + valid_iv_name(mrb, iv_name_id, s, len); } static mrb_sym -get_valid_iv_sym(mrb_state *mrb, mrb_value val) +get_valid_iv_sym(mrb_state *mrb, mrb_value iv_name) { - mrb_sym id; - - if (mrb_string_p(val)) { - id = mrb_intern_cstr(mrb, RSTRING_PTR(val)); - valid_iv_name(mrb, id, RSTRING_PTR(val), RSTRING_LEN(val)); - } else if(mrb_symbol_p(val)) { - id = mrb_symbol(val); - check_iv_name(mrb, id); + mrb_sym iv_name_id; + + if (mrb_string_p(iv_name)) { + iv_name_id = mrb_intern_cstr(mrb, RSTRING_PTR(iv_name)); + valid_iv_name(mrb, iv_name_id, RSTRING_PTR(iv_name), RSTRING_LEN(iv_name)); + } else if(mrb_symbol_p(iv_name)) { + iv_name_id = mrb_symbol(iv_name); + check_iv_name(mrb, iv_name_id); } - return id; + return iv_name_id; } /* 15.3.1.3.20 */ @@ -637,13 +637,13 @@ mrb_obj_ivar_defined(mrb_state *mrb, mrb_value self) mrb_value mrb_obj_ivar_get(mrb_state *mrb, mrb_value self) { - mrb_sym id; - mrb_value sym; + mrb_sym iv_name_id; + mrb_value iv_name; - mrb_get_args(mrb, "o", &sym); + mrb_get_args(mrb, "o", &iv_name); - id = get_valid_iv_sym(mrb, sym); - return mrb_iv_get(mrb, self, id); + iv_name_id = get_valid_iv_sym(mrb, iv_name); + return mrb_iv_get(mrb, self, iv_name_id); } /* 15.3.1.3.22 */ @@ -669,13 +669,13 @@ mrb_obj_ivar_get(mrb_state *mrb, mrb_value self) mrb_value mrb_obj_ivar_set(mrb_state *mrb, mrb_value self) { - mrb_sym id; - mrb_value sym, val; + mrb_sym iv_name_id; + mrb_value iv_name, val; - mrb_get_args(mrb, "oo", &sym, &val); + mrb_get_args(mrb, "oo", &iv_name, &val); - id = get_valid_iv_sym(mrb, sym); - mrb_iv_set(mrb, self, id, val); + iv_name_id = get_valid_iv_sym(mrb, iv_name); + mrb_iv_set(mrb, self, iv_name_id, val); return val; } -- cgit v1.2.3 From 43c0f43f1355c1d421b36f5ede7bb5c3aa6b6dd8 Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Sun, 21 Jul 2013 18:13:19 +1000 Subject: Changed the attr methods in mrblib, so that they take advantage of validating the name against a string parameter if need be --- mrblib/class.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mrblib/class.rb b/mrblib/class.rb index 0678d943d..6d624ee83 100644 --- a/mrblib/class.rb +++ b/mrblib/class.rb @@ -5,8 +5,8 @@ class Module name = name.to_s raise(NameError, "#{name.inspect} is not allowed as an instance variable name") if name.include?('@') || name.include?('?') || name.include?('$') - name2 = ('@'+name).intern - define_method(name){self.instance_variable_get(name2)} + attr_name = '@'+name + define_method(name){self.instance_variable_get(attr_name)} end end # 15.2.2.4.14 @@ -15,9 +15,9 @@ class Module name = name.to_s raise(NameError, "#{name.inspect} is not allowed as an instance variable name") if name.include?('@') || name.include?('?') || name.include?('$') - name2 = ('@'+name).intern + attr_name = '@'+name name = (name+"=").intern - define_method(name){|v|self.instance_variable_set(name2,v)} + define_method(name){|v|self.instance_variable_set(attr_name,v)} end end # 15.2.2.4.12 -- cgit v1.2.3 From cec3b3340362c1ba2343ca559531b095ad90133b Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Sun, 21 Jul 2013 18:26:01 +1000 Subject: Reset the so-render benchmark so that it is what it was before my attr performance testing. --- benchmark/ao-render.rb | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/benchmark/ao-render.rb b/benchmark/ao-render.rb index e7406b26b..793804a7b 100644 --- a/benchmark/ao-render.rb +++ b/benchmark/ao-render.rb @@ -6,8 +6,8 @@ # mruby version by Hideki Miura # -IMAGE_WIDTH = 64 -IMAGE_HEIGHT = 64 +IMAGE_WIDTH = 256 +IMAGE_HEIGHT = 256 NSUBSAMPLES = 2 NAO_SAMPLES = 8 @@ -30,14 +30,19 @@ module Rand end class Vec - attr_accessor :x, :y, :z - def initialize(x, y, z) @x = x @y = y @z = z end + def x=(v); @x = v; end + def y=(v); @y = v; end + def z=(v); @z = v; end + def x; @x; end + def y; @y; end + def z; @z; end + def vadd(b) Vec.new(@x + b.x, @y + b.y, @z + b.z) end @@ -75,13 +80,14 @@ end class Sphere - attr_reader :center, :radius - def initialize(center, radius) @center = center @radius = radius end + def center; @center; end + def radius; @radius; end + def intersect(ray, isect) rs = ray.org.vsub(@center) b = rs.vdot(ray.dir) @@ -134,23 +140,33 @@ class Plane end class Ray - attr_accessor :org, :dir - def initialize(org, dir) @org = org @dir = dir end + + def org; @org; end + def org=(v); @org = v; end + def dir; @dir; end + def dir=(v); @dir = v; end end class Isect - attr_accessor :t, :hit, :pl, :n - def initialize @t = 10000000.0 @hit = false @pl = Vec.new(0.0, 0.0, 0.0) @n = Vec.new(0.0, 0.0, 0.0) end + + def t; @t; end + def t=(v); @t = v; end + def hit; @hit; end + def hit=(v); @hit = v; end + def pl; @pl; end + def pl=(v); @pl = v; end + def n; @n; end + def n=(v); @n = v; end end def clamp(f) @@ -282,9 +298,9 @@ class Scene r = rad.x / (nsf * nsf) g = rad.y / (nsf * nsf) b = rad.z / (nsf * nsf) - # printf("%c", clamp(r)) - # printf("%c", clamp(g)) - # printf("%c", clamp(b)) + printf("%c", clamp(r)) + printf("%c", clamp(g)) + printf("%c", clamp(b)) end end end -- cgit v1.2.3 From ab787ab9683b8295ab2cb03d94dbbad1ebfe5db7 Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Sun, 21 Jul 2013 18:43:00 +1000 Subject: Cleaned up some of the Module#attr tests --- test/t/module.rb | 77 +++++++++++++------------------------------------------- 1 file changed, 18 insertions(+), 59 deletions(-) diff --git a/test/t/module.rb b/test/t/module.rb index a03511b09..618b57f06 100644 --- a/test/t/module.rb +++ b/test/t/module.rb @@ -37,34 +37,43 @@ assert('Module#append_features', '15.2.2.4.10') do assert_equal Test4AppendFeatures2.const_get(:Const4AppendFeatures2), Test4AppendFeatures2 end -assert('Module#attr', '15.2.2.4.11') do +assert('Module#attr NameError') do %w[ foo? @foo @@foo $foo ].each do |name| + module NameTest; end + assert_raise(NameError) do - module NameTest; end NameTest.module_eval { attr_reader name.to_sym } end + + assert_raise(NameError) do + NameTest.module_eval { attr_writer name.to_sym } + end + + assert_raise(NameError) do + NameTest.module_eval { attr name.to_sym } + end + + assert_raise(NameError) do + NameTest.module_eval { attr_accessor name.to_sym } + end end +end + +assert('Module#attr', '15.2.2.4.11') do class AttrTest class << self attr :cattr - def cattr_val - @cattr - end def cattr_val=(val) @cattr = val end end attr :iattr - - def iattr_val - @iattr - end def iattr_val=(val) @iattr = val end @@ -85,18 +94,6 @@ assert('Module#attr', '15.2.2.4.11') do end assert('Module#attr_accessor', '15.2.2.4.12') do - %w[ - foo? - @foo - @@foo - $foo - ].each do |name| - assert_raise(NameError) do - module NameTest; end - NameTest.module_eval { attr_reader name.to_sym } - end - end - class AttrTestAccessor class << self attr_accessor :cattr @@ -120,33 +117,14 @@ assert('Module#attr_accessor', '15.2.2.4.12') do end assert('Module#attr_reader', '15.2.2.4.13') do - %w[ - foo? - @foo - @@foo - $foo - ].each do |name| - assert_raise(NameError) do - module NameTest; end - NameTest.module_eval { attr_reader name.to_sym } - end - end - class AttrTestReader class << self attr_reader :cattr - def cattr_val - @cattr - end def cattr_val=(val) @cattr = val end end attr_reader :iattr, 'iattr2' - - def iattr_val - @iattr - end def iattr_val=(val) @iattr = val end @@ -169,36 +147,17 @@ assert('Module#attr_reader', '15.2.2.4.13') do end assert('Module#attr_writer', '15.2.2.4.14') do - %w[ - foo? - @foo - @@foo - $foo - ].each do |name| - assert_raise(NameError) do - module NameTest; end - NameTest.module_eval { attr_reader name.to_sym } - end - end - class AttrTestWriter class << self attr_writer :cattr def cattr_val @cattr end - def cattr_val=(val) - @cattr = val - end end attr_writer :iattr, 'iattr2' - def iattr_val @iattr end - def iattr_val=(val) - @iattr = val - end end attr_instance = AttrTestWriter.new -- cgit v1.2.3