Commit 36e01a77 authored by oliver@apple.com's avatar oliver@apple.com
Browse files

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<StringImpl>. 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<RefPtr<...>, ...>
specialization, because the transition table is actually
HashMap<pair<RefPtr<StringImpl>, unsigned>, ...>: 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):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@153141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent cece4b66
2013-05-02 Filip Pizlo <fpizlo@apple.com>
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<StringImpl>. 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<RefPtr<...>, ...>
specialization, because the transition table is actually
HashMap<pair<RefPtr<StringImpl>, unsigned>, ...>: 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):
2013-05-01 Filip Pizlo <fpizlo@apple.com>
fourthTier: Structure::addPropertyTransitionToExistingStructure should be thread-safe
......
......@@ -103,7 +103,7 @@ inline void StructureTransitionTable::add(VM& vm, Structure* structure)
// Newer versions of the STL have an std::make_pair function that takes rvalue references.
// 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 "+" to make the parameter an rvalue.
// See https://bugs.webkit.org/show_bug.cgi?id=59261 for more details
map()->set(make_pair(structure->m_nameInPrevious, +structure->m_attributesInPrevious), structure);
map()->set(make_pair(structure->m_nameInPrevious.get(), +structure->m_attributesInPrevious), structure);
}
void Structure::dumpStatistics()
......
/*
* Copyright (C) 2008, 2009 Apple Inc. All rights reserved.
* Copyright (C) 2008, 2009, 2013 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
......@@ -30,7 +30,6 @@
#include "WeakGCMap.h"
#include <wtf/HashFunctions.h>
#include <wtf/OwnPtr.h>
#include <wtf/RefPtr.h>
#include <wtf/text/StringImpl.h>
namespace JSC {
......@@ -93,14 +92,13 @@ inline IndexingType newIndexingType(IndexingType oldType, NonPropertyTransition
class StructureTransitionTable {
static const intptr_t UsingSingleSlotFlag = 1;
struct Hash {
typedef std::pair<RefPtr<StringImpl>, unsigned> Key;
typedef std::pair<StringImpl*, unsigned> Key;
static unsigned hash(const Key& p)
{
unsigned result = p.second;
if (p.first)
result += p.first->existingHash();
return result;
return PtrHash<StringImpl*>::hash(p.first) + p.second;
}
static bool equal(const Key& a, const Key& b)
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment