https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112018-02-14T20:40:47ZRuby Issue Tracking SystemRuby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703492018-02-14T20:40:47Zsam.saffron (Sam Saffron)sam.saffron@gmail.com
<ul></ul><p>Simpler repro:</p>
<pre><code>x = "#{1}".freeze
p x.object_id
x = -x
p x.object_id
x = "#{1}".freeze
p x.object_id
x = -x
p x.object_id
</code></pre>
<pre><code>70210401536460
70210401536460
70210401536240
70210401536240
</code></pre> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703502018-02-14T20:43:41Zsam.saffron (Sam Saffron)sam.saffron@gmail.com
<ul></ul><p>Workaround ... whenever de-duping strings always use <code>-+</code></p>
<pre><code>x = "#{1}".freeze
# this is always properly de-duped
x = -+x
</code></pre>
<p>Any objections to changing:</p>
<pre><code>static VALUE
str_uminus(VALUE str)
{
if (OBJ_FROZEN(str)) {
return str;
}
else {
return rb_fstring(str);
}
}
</code></pre>
<p>To</p>
<pre><code>static VALUE
str_uminus(VALUE str)
{
return rb_fstring(str);
}
</code></pre> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703512018-02-14T22:12:23Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:sam.saffron@gmail.com" class="email">sam.saffron@gmail.com</a> wrote:</p>
<blockquote>
<p>Workaround ... whenever de-duping strings always use <code>-+</code></p>
</blockquote>
<p>Yes, String#-@ documentation states:</p>
<p>"If the string is frozen, then return the string itself."</p>
<p>So I guess it is spec...</p>
<p>However, it seems compile-time can skip freezestring instructions<br>
for -@ like it does with +@ since r62039</p>
<pre><code>diff --git a/compile.c b/compile.c
index 5bca0dca99..7d065b25d3 100644
--- a/compile.c
+++ b/compile.c
@@ -2851,11 +2851,11 @@ iseq_peephole_optimize(rb_iseq_t *iseq, LINK_ELEMENT *list, const int do_tailcal
struct rb_call_info *ci = (struct rb_call_info *)OPERAND_AT(niobj, 0);
/*
* freezestring debug_info
- * send <:+@, 0, ARG_SIMPLE>
+ * send <:+@, 0, ARG_SIMPLE> # :-@, too
* =>
- * send <:+@, 0, ARG_SIMPLE>
+ * send <:+@, 0, ARG_SIMPLE> # :-@, too
*/
- if (ci->mid == idUPlus &&
+ if ((ci->mid == idUPlus || ci->mid == idUMinus) &&
(ci->flag & VM_CALL_ARGS_SIMPLE) &&
ci->orig_argc == 0) {
ELEM_REMOVE(list);
diff --git a/test/ruby/test_optimization.rb b/test/ruby/test_optimization.rb
index 6e463e1863..55e7acbbf0 100644
--- a/test/ruby/test_optimization.rb
+++ b/test/ruby/test_optimization.rb
@@ -567,6 +567,21 @@ def test_peephole_string_literal_range
end
end
+ def test_peephole_dstr
+ code = "#{<<~'begin;'}\n#{<<~'end;'}"
+ begin;
+ exp = (-'a').object_id
+ z = 'a'
+ exp == (-"#{z}").object_id
+ end;
+ [ false, true ].each do |fsl|
+ iseq = RubyVM::InstructionSequence.compile(code,
+ frozen_string_literal: fsl)
+ assert_equal(true, iseq.eval,
+ "<a href="/issues/14475">[ruby-core:85542]</a> [Bug #14475] fsl: #{fsl}")
+ end
+ end
+
def test_branch_condition_backquote
bug = '<a href="/issues/13444">[ruby-core:80740]</a> [Bug #13444] redefined backquote should be called'
class << self
</code></pre> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703522018-02-14T22:27:50Zsam.saffron (Sam Saffron)sam.saffron@gmail.com
<ul></ul><p>"If the string is frozen, then return the string itself."</p>
<p>Yeah I do not agree with this documentation, I think it should be changed.</p>
<p>Trouble is that there is no simple way to de-duplicate unconditionally without inefficiency:</p>
<p>Say <code>x</code> is an arbitrary string (either frozen or unfrozen) :</p>
<pre><code>x = -x # may return a non fstring
x = -+x # will return fstring, but makes an unneeded copy
x = -x.dup # fstring again, uneeded copy
x = x.frozen? ? -+x : -x # too verbose, uneeded copy
</code></pre>
<p>Instead why not change it so <code>-</code> is deduped unconditionally?</p> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703542018-02-14T22:35:16ZAnonymous
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Applied in changeset trunk|r62407.</p>
<hr>
<p>compile.c: drop freezestring insn on String#-@</p>
<p>Followup to r62039 and remove the redundant freezestring<br>
insn which was preventing deduplication from String#-@</p>
<ul>
<li>compile.c (iseq_peephole_optimize): drop freezestring insn on String#-@<br>
<a href="/issues/14475">[ruby-core:85542]</a> [Bug <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: String de-duplication is broken in files with frozen_string_literal: true (Closed)" href="https://bugs.ruby-lang.org/issues/14475">#14475</a>]</li>
</ul> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703552018-02-14T22:45:46Zsam.saffron (Sam Saffron)sam.saffron@gmail.com
<ul></ul><p>Opened <a href="https://bugs.ruby-lang.org/issues/14478" class="external">https://bugs.ruby-lang.org/issues/14478</a> to discuss change to uminus</p> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703562018-02-14T22:52:11Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:sam.saffron@gmail.com" class="email">sam.saffron@gmail.com</a> wrote:</p>
<blockquote>
<p>"If the string is frozen, then return the string itself."</p>
<p>Yeah I do not agree with this documentation, I think it should be changed.</p>
<p>Trouble is that there is no simple way to de-duplicate<br>
unconditionally without either inefficiency or side effects:</p>
</blockquote>
<p>Yeah, I originally proposed a new method back in the day but<br>
compromised with String#-@ <a href="https://bugs.ruby-lang.org/issues/13077" class="external">https://bugs.ruby-lang.org/issues/13077</a></p>
<blockquote>
<p>Say <code>x</code> is an arbitrary string (either frozen or unfrozen) :</p>
<pre><code>x = -x # may return a non fstring
x = -+x # will return fstring, but makes an unneeded copy
x = -x.dup # fstring again, uneeded copy
</code></pre>
</blockquote>
<p>Maybe these can be optimized in the VM (still ugly, and<br>
redefinition checks aren't free, either).</p>
<blockquote>
<p>x = x.frozen? ? -+x : -x # too verbose</p>
<pre><code>
Instead why not change it so `-` deduped unconditionally?
</code></pre>
</blockquote>
<p>At this point, it may break existing code :<</p> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703592018-02-14T23:42:21Zsam.saffron (Sam Saffron)sam.saffron@gmail.com
<ul></ul><p>I created a new topic for this at: <a href="https://bugs.ruby-lang.org/issues/14478" class="external">https://bugs.ruby-lang.org/issues/14478</a></p>
<blockquote>
<p>At this point, it may break existing code :<</p>
</blockquote>
<p>I think it is worth the risk cause adding #fstring here seem overkill and almost nobody would be using uminus now anyway cause they do not know about it.</p> Ruby master - Bug #14475: String de-duplication is broken in files with frozen_string_literal: truehttps://bugs.ruby-lang.org/issues/14475?journal_id=703632018-02-15T01:39:25Zduerst (Martin Dürst)duerst@it.aoyama.ac.jp
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-5 priority-4 priority-default closed" href="/issues/14478">Feature #14478</a>: String #uminus should de-dupe unconditionally</i> added</li></ul>