<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[149512] branches/dfgFourthTier/Source/JavaScriptCore</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/149512">149512</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2013-05-02 17:09:39 -0700 (Thu, 02 May 2013)</dd>
</dl>

<h3>Log Message</h3>
<pre>fourthTier: Structure transition table keys don't have to ref their StringImpl's
https://bugs.webkit.org/show_bug.cgi?id=115525

Reviewed by Geoffrey Garen.
        
The structure transition table basically maps string to structure. The string is
always also stored, and ref'd, in the structure in Structure::m_nameInPrevious.
m_nameInPrevious is never mutated, and never cleared. The string cannot die unless
the structure dies. If the structure dies, then that entry in the transition map
becomes a zombie anyway and we will detect this separately.
        
So, we don't need to use RefPtr&lt;StringImpl&gt;. We can just use StringImpl*.
        
This also fixes a goof where we were getting the StringImpl's hash rather than
using a pointer hash. Not only is the latter faster, but it prevents my change
from leading to crashes: with my change we can have zombie keys, not just zombie
values. They will exist only until the next map mutation, which will clear them.
Lookups will work fine because the lookup routine will reject zombies. But it
does mean that the HashMap will have to deal with dangling StringImpl*'s; all it
takes to make this work is to ensure that the HashMap itself never dereferences
them. Using a pointer hash rather than StringImpl::existingHash() accomplishes
this.
        
This also ensures that we don't accidentally call ref() or deref() from the
compilation thread, if the compilation thread inspects the transition table.
        
And no, we wouldn't have been able to use the HashMap&lt;RefPtr&lt;...&gt;, ...&gt;
specialization, because the transition table is actually
HashMap&lt;pair&lt;RefPtr&lt;StringImpl&gt;, unsigned&gt;, ...&gt;: hence that specialization
doesn't kick in. We could have written a new specialization or something, but
that seemed like a lot of work given that we don't need the table to be ref'ing
the strings anyways.

* runtime/Structure.cpp:
(JSC::StructureTransitionTable::add):
* runtime/StructureTransitionTable.h:
(StructureTransitionTable):
(Hash):
(JSC::StructureTransitionTable::Hash::hash):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#branchesdfgFourthTierSourceJavaScriptCoreChangeLog">branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#branchesdfgFourthTierSourceJavaScriptCoreruntimeStructurecpp">branches/dfgFourthTier/Source/JavaScriptCore/runtime/Structure.cpp</a></li>
<li><a href="#branchesdfgFourthTierSourceJavaScriptCoreruntimeStructureTransitionTableh">branches/dfgFourthTier/Source/JavaScriptCore/runtime/StructureTransitionTable.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="branchesdfgFourthTierSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog (149511 => 149512)</h4>
<pre class="diff"><span>
<span class="info">--- branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog        2013-05-03 00:04:34 UTC (rev 149511)
+++ branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog        2013-05-03 00:09:39 UTC (rev 149512)
</span><span class="lines">@@ -1,3 +1,45 @@
</span><ins>+2013-05-02  Filip Pizlo  &lt;fpizlo@apple.com&gt;
+
+        fourthTier: Structure transition table keys don't have to ref their StringImpl's
+        https://bugs.webkit.org/show_bug.cgi?id=115525
+
+        Reviewed by Geoffrey Garen.
+        
+        The structure transition table basically maps string to structure. The string is
+        always also stored, and ref'd, in the structure in Structure::m_nameInPrevious.
+        m_nameInPrevious is never mutated, and never cleared. The string cannot die unless
+        the structure dies. If the structure dies, then that entry in the transition map
+        becomes a zombie anyway and we will detect this separately.
+        
+        So, we don't need to use RefPtr&lt;StringImpl&gt;. We can just use StringImpl*.
+        
+        This also fixes a goof where we were getting the StringImpl's hash rather than
+        using a pointer hash. Not only is the latter faster, but it prevents my change
+        from leading to crashes: with my change we can have zombie keys, not just zombie
+        values. They will exist only until the next map mutation, which will clear them.
+        Lookups will work fine because the lookup routine will reject zombies. But it
+        does mean that the HashMap will have to deal with dangling StringImpl*'s; all it
+        takes to make this work is to ensure that the HashMap itself never dereferences
+        them. Using a pointer hash rather than StringImpl::existingHash() accomplishes
+        this.
+        
+        This also ensures that we don't accidentally call ref() or deref() from the
+        compilation thread, if the compilation thread inspects the transition table.
+        
+        And no, we wouldn't have been able to use the HashMap&lt;RefPtr&lt;...&gt;, ...&gt;
+        specialization, because the transition table is actually
+        HashMap&lt;pair&lt;RefPtr&lt;StringImpl&gt;, unsigned&gt;, ...&gt;: hence that specialization
+        doesn't kick in. We could have written a new specialization or something, but
+        that seemed like a lot of work given that we don't need the table to be ref'ing
+        the strings anyways.
+
+        * runtime/Structure.cpp:
+        (JSC::StructureTransitionTable::add):
+        * runtime/StructureTransitionTable.h:
+        (StructureTransitionTable):
+        (Hash):
+        (JSC::StructureTransitionTable::Hash::hash):
+
</ins><span class="cx"> 2013-05-01  Filip Pizlo  &lt;fpizlo@apple.com&gt;
</span><span class="cx"> 
</span><span class="cx">         fourthTier: Structure::addPropertyTransitionToExistingStructure should be thread-safe
</span></span></pre></div>
<a id="branchesdfgFourthTierSourceJavaScriptCoreruntimeStructurecpp"></a>
<div class="modfile"><h4>Modified: branches/dfgFourthTier/Source/JavaScriptCore/runtime/Structure.cpp (149511 => 149512)</h4>
<pre class="diff"><span>
<span class="info">--- branches/dfgFourthTier/Source/JavaScriptCore/runtime/Structure.cpp        2013-05-03 00:04:34 UTC (rev 149511)
+++ branches/dfgFourthTier/Source/JavaScriptCore/runtime/Structure.cpp        2013-05-03 00:09:39 UTC (rev 149512)
</span><span class="lines">@@ -103,7 +103,7 @@
</span><span class="cx">     // Newer versions of the STL have an std::make_pair function that takes rvalue references.
</span><span class="cx">     // When either of the parameters are bitfields, the C++ compiler will try to bind them as lvalues, which is invalid. To work around this, use unary &quot;+&quot; to make the parameter an rvalue.
</span><span class="cx">     // See https://bugs.webkit.org/show_bug.cgi?id=59261 for more details
</span><del>-    map()-&gt;set(make_pair(structure-&gt;m_nameInPrevious, +structure-&gt;m_attributesInPrevious), structure);
</del><ins>+    map()-&gt;set(make_pair(structure-&gt;m_nameInPrevious.get(), +structure-&gt;m_attributesInPrevious), structure);
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void Structure::dumpStatistics()
</span></span></pre></div>
<a id="branchesdfgFourthTierSourceJavaScriptCoreruntimeStructureTransitionTableh"></a>
<div class="modfile"><h4>Modified: branches/dfgFourthTier/Source/JavaScriptCore/runtime/StructureTransitionTable.h (149511 => 149512)</h4>
<pre class="diff"><span>
<span class="info">--- branches/dfgFourthTier/Source/JavaScriptCore/runtime/StructureTransitionTable.h        2013-05-03 00:04:34 UTC (rev 149511)
+++ branches/dfgFourthTier/Source/JavaScriptCore/runtime/StructureTransitionTable.h        2013-05-03 00:09:39 UTC (rev 149512)
</span><span class="lines">@@ -1,5 +1,5 @@
</span><span class="cx"> /*
</span><del>- * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2008, 2009, 2013 Apple Inc. All rights reserved.
</ins><span class="cx">  *
</span><span class="cx">  * Redistribution and use in source and binary forms, with or without
</span><span class="cx">  * modification, are permitted provided that the following conditions
</span><span class="lines">@@ -30,7 +30,6 @@
</span><span class="cx"> #include &quot;WeakGCMap.h&quot;
</span><span class="cx"> #include &lt;wtf/HashFunctions.h&gt;
</span><span class="cx"> #include &lt;wtf/OwnPtr.h&gt;
</span><del>-#include &lt;wtf/RefPtr.h&gt;
</del><span class="cx"> #include &lt;wtf/text/StringImpl.h&gt;
</span><span class="cx"> 
</span><span class="cx"> namespace JSC {
</span><span class="lines">@@ -93,14 +92,13 @@
</span><span class="cx"> class StructureTransitionTable {
</span><span class="cx">     static const intptr_t UsingSingleSlotFlag = 1;
</span><span class="cx"> 
</span><ins>+    
</ins><span class="cx">     struct Hash {
</span><del>-        typedef std::pair&lt;RefPtr&lt;StringImpl&gt;, unsigned&gt; Key;
</del><ins>+        typedef std::pair&lt;StringImpl*, unsigned&gt; Key;
+        
</ins><span class="cx">         static unsigned hash(const Key&amp; p)
</span><span class="cx">         {
</span><del>-            unsigned result = p.second;
-            if (p.first)
-                result += p.first-&gt;existingHash();
-            return result;
</del><ins>+            return PtrHash&lt;StringImpl*&gt;::hash(p.first) + p.second;
</ins><span class="cx">         }
</span><span class="cx"> 
</span><span class="cx">         static bool equal(const Key&amp; a, const Key&amp; b)
</span></span></pre>
</div>
</div>

</body>
</html>