Project

General

Profile

Actions
Copy link

Bug #15118

closed

Method [] & []= does not respect frozen_string_literal: true comment

Bug #15118: Method [] & []= does not respect frozen_string_literal: true comment

Added by chopraanmol1 (Anmol Chopra) about 7 years ago. Updated about 7 years ago.

Status:
Closed
Assignee:
-
Target version:
-
ruby -v:
ruby 2.6.0dev (2018-09-13 trunk 64736) [x86_64-linux]
Backport:
2.3: REQUIRED, 2.4: DONE, 2.5: DONE
[ruby-core:89003]

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 7 years ago Actions
Copy link
 #1

  • Description updated (diff)

Updated by shevegen (Robert A. Heiler) about 7 years ago Actions
Copy link
 #2 [ruby-core:89005]

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 7 years ago Actions
Copy link
 #3 [ruby-core:89007]

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 7 years ago Actions
Copy link
 #4

  • Description updated (diff)

Updated by chopraanmol1 (Anmol Chopra) about 7 years ago Actions
Copy link
 #5

  • Subject changed from Method [] does not respect frozen_string_literal: true comment to Method [] & []= does not respect frozen_string_literal: true comment

Updated by Hanmac (Hans Mackowiak) about 7 years ago Actions
Copy link
 #7 [ruby-core:89040]

@nobu (Nobuyoshi Nakada) : should this ticket be closed or not yet?

it seems fixed in 64745

Updated by nobu (Nobuyoshi Nakada) about 7 years ago Actions
Copy link
 #8 [ruby-core:89045]

  • 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 7 years ago Actions
Copy link
 #9 [ruby-core:89375]

  • 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 7 years ago Actions
Copy link
 #10 [ruby-core:89441]

  • 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.

Actions
Copy link

Also available in: PDF Atom