Bug #15118
closedMethod [] & []= does not respect frozen_string_literal: true comment
Description
Calling ["something"]
on object or proc (non-hash aref implementation) does not respect frozen_string_literal: true
comment
Script:¶
# frozen_string_literal: true
require 'benchmark'
require 'memory_profiler'
class NopId
def self.[](str)
str.__id__
end
def self.[]=(str, val)
str.__id__
end
end
SampleHash = {"sometext" => 0}
NopProc = proc{|a| a.__id__}
N = 1_000_000
def method1
NopId["sometext"]
end
def method2
SampleHash["sometext"]
end
def method3
NopProc["sometext"]
end
def method4
NopId["sometext"] = 'othertext'
end
def print_iseq method_name
puts "-+"*20
puts RubyVM::InstructionSequence.disasm method(method_name)
puts "-+"*20
end
def print_memory_profiler title, &block
puts "-+"*20
puts title
MemoryProfiler.report{N.times(&block)}.pretty_print(detailed_report: false, allocated_strings: 0, retained_strings: 0)
puts "-+"*20
end
print_iseq :method1
print_iseq :method2
print_iseq :method3
print_iseq :method4
Benchmark.bm(10) do |bm|
bm.report("method[]"){ N.times{ method1 } }
bm.report("hash[]"){ N.times{ method2 } }
bm.report("proc[]"){ N.times{ method3 } }
bm.report("method[]="){ N.times{ method4 } }
end
print_memory_profiler("method[]"){method1}
print_memory_profiler("hash[]"){method2}
print_memory_profiler("proc[]"){method3}
print_memory_profiler("method[]="){method4}
Output:¶
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method1@../test_aref.rb:21 (21,0)-(23,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 22)[LiCa]
0003 getconstant :NopId
0005 setinlinecache <is:0>
0007 opt_aref_with "sometext", <callinfo!mid:[], argc:1, ARGS_SIMPLE>, <callcache>
0011 leave ( 23)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method2@../test_aref.rb:25 (25,0)-(27,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 26)[LiCa]
0003 getconstant :SampleHash
0005 setinlinecache <is:0>
0007 opt_aref_with "sometext", <callinfo!mid:[], argc:1, ARGS_SIMPLE>, <callcache>
0011 leave ( 27)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method3@../test_aref.rb:29 (29,0)-(31,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 30)[LiCa]
0003 getconstant :NopProc
0005 setinlinecache <is:0>
0007 opt_aref_with "sometext", <callinfo!mid:[], argc:1, ARGS_SIMPLE>, <callcache>
0011 leave ( 31)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method4@../test_aref.rb:33 (33,0)-(35,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 34)[LiCa]
0003 getconstant :NopId
0005 setinlinecache <is:0>
0007 putobject "othertext"
0009 swap
0010 topn 1
0012 opt_aset_with "sometext", <callinfo!mid:[]=, argc:2, ARGS_SIMPLE>, <callcache>
0016 pop
0017 leave ( 35)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
user system total real
method[] 0.120000 0.004000 0.124000 ( 0.121883)
hash[] 0.088000 0.000000 0.088000 ( 0.088723)
proc[] 0.132000 0.000000 0.132000 ( 0.133687)
method[]= 0.128000 0.000000 0.128000 ( 0.126702)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
method[]
Total allocated: 40000000 bytes (1000000 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
hash[]
Total allocated: 0 bytes (0 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
proc[]
Total allocated: 40000000 bytes (1000000 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
method[]=
Total allocated: 40000000 bytes (1000000 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
As you can observe calling NopClass["something"]
& NopProc["something"]
does not respect frozen_string_literal: true
comment
Patch:¶
https://github.com/ruby/ruby/pull/1957
After Patch Result:¶
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method1@../test_aref.rb:21 (21,0)-(23,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 22)[LiCa]
0003 getconstant :NopId
0005 setinlinecache <is:0>
0007 putobject "sometext"
0009 opt_aref <callinfo!mid:[], argc:1, ARGS_SIMPLE>, <callcache>
0012 leave ( 23)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method2@../test_aref.rb:25 (25,0)-(27,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 26)[LiCa]
0003 getconstant :SampleHash
0005 setinlinecache <is:0>
0007 putobject "sometext"
0009 opt_aref <callinfo!mid:[], argc:1, ARGS_SIMPLE>, <callcache>
0012 leave ( 27)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method3@../test_aref.rb:29 (29,0)-(31,3)> (catch: FALSE)
0000 getinlinecache 7, <is:0> ( 30)[LiCa]
0003 getconstant :NopProc
0005 setinlinecache <is:0>
0007 putobject "sometext"
0009 opt_aref <callinfo!mid:[], argc:1, ARGS_SIMPLE>, <callcache>
0012 leave ( 31)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
== disasm: #<ISeq:method4@../test_aref.rb:33 (33,0)-(35,3)> (catch: FALSE)
0000 putnil ( 34)[LiCa]
0001 getinlinecache 8, <is:0>
0004 getconstant :NopId
0006 setinlinecache <is:0>
0008 putobject "sometext"
0010 putobject "othertext"
0012 setn 3
0014 opt_aset <callinfo!mid:[]=, argc:2, ARGS_SIMPLE>, <callcache>
0017 pop
0018 leave ( 35)[Re]
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
user system total real
method[] 0.092000 0.004000 0.096000 ( 0.094751)
hash[] 0.088000 0.000000 0.088000 ( 0.087865)
proc[] 0.116000 0.000000 0.116000 ( 0.117047)
method[]= 0.096000 0.000000 0.096000 ( 0.096765)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
method[]
Total allocated: 0 bytes (0 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
hash[]
Total allocated: 0 bytes (0 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
proc[]
Total allocated: 0 bytes (0 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
method[]=
Total allocated: 0 bytes (0 objects)
Total retained: 0 bytes (0 objects)
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Updated by chopraanmol1 (Anmol Chopra) about 6 years ago
- Description updated (diff)
Updated by shevegen (Robert A. Heiler) about 6 years ago
Interesting.
I just tested with this code:
#!/System/Index/bin/ruby -w
# Encoding: ISO-8859-1
# frozen_string_literal: true
# =========================================================================== #
NopProc = proc {|a| a }
def foobar
result = NopProc["sometext"]
result
end
x = foobar
puts x.class
puts x.frozen?
y = 'abc'
puts y.frozen?
# Result:
# String
# false
# true
And I think you are right. I am testing .frozen?
twice; the 'abc'
string is indeed frozen whereas the variant returned by []
does
not seem to honour the instruction in the "magic" comment section.
Very good catch. May I ask, for curiosity, how you discovered it?
I assume you may have tested somehow systematically or something?
Updated by chopraanmol1 (Anmol Chopra) about 6 years ago
shevegen (Robert A. Heiler) wrote:
Very good catch. May I ask, for curiosity, how you discovered it?
I assume you may have tested somehow systematically or something?
I was working on reducing memory allocation on roo gem. While profiling (with memory_profiler gem) I observed string literal being allocated multiple time at specific locations even after adding magic string literal comment. After debugging for while I discovered calling this method https://github.com/sparklemotion/nokogiri/blob/7b8cd0f5b15a926e92c869b450dd6f71cdd17b61/lib/nokogiri/xml/node.rb#L120 in cell_xml['r'] fashion resulted into above behavior.
Updated by chopraanmol1 (Anmol Chopra) about 6 years ago
- Description updated (diff)
Updated by chopraanmol1 (Anmol Chopra) about 6 years ago
- Subject changed from Method [] does not respect frozen_string_literal: true comment to Method [] & []= does not respect frozen_string_literal: true comment
Updated by chopraanmol1 (Anmol Chopra) about 6 years ago
Updated by Hanmac (Hans Mackowiak) about 6 years ago
@nobu (Nobuyoshi Nakada) : should this ticket be closed or not yet?
it seems fixed in 64745
Updated by nobu (Nobuyoshi Nakada) about 6 years ago
- Description updated (diff)
- Status changed from Open to Closed
- Backport changed from 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN to 2.3: REQUIRED, 2.4: REQUIRED, 2.5: REQUIRED
Thank you, I missed the reference to this ticket in the commit log.
Updated by nagachika (Tomoyuki Chikanaga) about 6 years ago
- Backport changed from 2.3: REQUIRED, 2.4: REQUIRED, 2.5: REQUIRED to 2.3: REQUIRED, 2.4: REQUIRED, 2.5: DONE
ruby_2_5 r65002 merged revision(s) 64745.
Updated by usa (Usaku NAKAMURA) about 6 years ago
- Backport changed from 2.3: REQUIRED, 2.4: REQUIRED, 2.5: DONE to 2.3: REQUIRED, 2.4: DONE, 2.5: DONE
ruby_2_4 r65116 merged revision(s) 64745.