PolicyArray pased all tests but failed in production. After browsing on my own box I figured that what happened was the method addLval() with a string key is called in production on many pages, but never in the tests. @ptarjan maybe you could look into adding such a test?
This diff still does not instantiate the policy-based array (I did instantiate it during testing).
I was learning from @jdelong and he said that you should use
double quotes for local includes and angle brackets for library
includes. I asked why our code was the way it was, and he said he wanted
to clean it up. I beat him to it :)
Conflicts:
hphp/runtime/base/server/admin_request_handler.cpp
hphp/runtime/vm/named_entity.h
PolicyArray splits the ArrayData implementation in two parts. ArrayShell implements the baroque ArrayData implementation in terms of a much smaller statically-bound core that concerns itself exclusively with the storage strategy for the array. This way the two aspects can be worked on separately, and different stores can be easily plugged into the given ArrayShell.
To give a better perspective, the featured SimpleArrayStore has about 340 lines all told (including solid documentation), whereas ArrayShell has some 1100 lines. The shell can be reused with other stores of unbounded sophistication. The store needs to implement only 22 primitives, some of which are trivial. Basically a new store implementation saves 1100 of difficult-to-get-right lines of code right off the bat, and only needs to focus on implementing 22 well-defined primitives.
Things to watch for when reviewing:
- Is the store API small/expressive enough? What should be added/removed?
- Are various forms of duplication present? If so, how could code be factored better?
- Any subtle change in semantics from HphpArray and friends that should be minded?
Known issues:
* Currently ArrayShell is defined as:
class ArrayShell : public ArrayData, private SimpleArrayStore { ... };
when in fact it should be:
template <class StorePolicy>
class ArrayShell : public ArrayData, private StorePolicy { ... };
Extenuating circumstances related to allocator design prevent use of templates at this point. I'll work on these in parallel with the review.
* growNoResize is almost always prefaced by a test-and-grow. This sequence should be encoded in a function.
* The growth policy is spread all over the place in a subtle form of duplication.
* The current store design makes almost no effort to be particularly efficient.
We can't box a non-ref value for a ref param because
we could be modifying an array with refCount != 1.
zend warns, skips the call and returns null. For now, this
makes us warn, but do the call (without modifying the array).
A file scope flag controls whether to skip the call or not,
and should be changed (and eliminated) once all our tests pass.
With the flag turned on, we still dont match zend's behavior,
because its happy to go ahead and call the function with no
warning in the case of a literal array parameter
(cuf('foo', array(1))), even though it warns and skips it when
the array is in a variable ($a=array(1); cuf('foo', $a)).
This is a partial step towards merging the HPHP::VM namespace
up into its parent. To keep it reviewable/mergeable I'm not doing
everything at once here, but most of the code I've touched seems
improved. I've drawn an invisible line around the jit, Unit and
its cohort (Class, Func, PreClass, etc.); we'll get back to them
soon.
Make the targetcache the one true home for constants,
so we dont need to (also) insert them all into
VMExecutionContext::m_constants (which is now gone).
By also making "non-volatile" constants persistent, we save
initializing most of them at all in RepoAuthoritative mode.
Streamline the array access methods by always returning an array
pointer instead of a new pointer or null. Callsites compare
(new != old) to detect escalation, rather than (new != null).
While I was working on the TestCodeRun refactor I found two tests about Tainted code. I looked into it and coulnd't get HHVM to compile with TAINTED=1. Then I checked and none of the extension functions we exposed about tainting were used in WWW. Scratching my head I asked, @srenfro and @jdelong, who thought it was dead. So I killed this zombie.
Replaces the awkward getFullPos() and setFullPos() methods with a more
intuitive advanceFullPos() method. This refactoring also reduces the number
of virtual calls made when doing mutable iteration on an array.
unserialize() and call_user_func_array() were straightforward. They were
called from all over the runtime, but I renamed those implementations
and codemodded the runtime.
The is_* functions were only ever being called by the CVarRef signature,
so I deleted all the other ones (same for f_gettype). Only some of the
is_* functions were being called from the runtime, so I made inline
versions of those without the f_ prefix.
Fixing various bugs all over the VM that make assumptions about RefData
and TypedValue layout. Here are the assumptions fixed by this diff:
offsetof(RefData, m_tv) == 0. Both JIT's assumed this in many subtle
ways, by punning RefData* as TypedValue* without adding an offset.
This assumption also causes RefData._count to overlap TypedValue.m_aux,
which constraints TypedValue layout.
offsetof(TypedValue, m_data) == 0. gen_ext_hhvm.php assumes you
can cast TypedValue* to Value*; the JITs often weren't using
offsetof(TypedValue, m_data) in their addressing calculations. HHIR
assumed return-by-value TV's have m_data/m_type in rax/rdx, which
can change when TV layout changes.
offsetof(TypedValue, m_type) > 8 is an assumption baked into the
pass-by-value register assignment logic in HHIR's codegen.cpp; if
the type is in the low word, register assignment is swapped.
sizeof(TypedValue::m_type) == 4. We used dword-sized operations
in both JIT's when accessing m_type. Now, we use helper functions
that are sensitive to sizeof(DataType)
Configuration:
DEBUG=: (opt) same layouts as trunk for RefData & TypedValue
DEBUG=1: (dbg) new RefData layout (m_tv doesn't overlap RefData::_count)
PACKED_TV=1, DEBUG=*: new RefData and TypedValue layout.
Of the four horsemen of the SmartAllocator, ArrayData was the only virtual
call. This meant an extra layer of indirection when coming from the TC
to allow the c++ compiler to emit its virtual call, and slightly larger
callsites when using non-generic paths.
While we're moving in this direction, consolidate ArrayData introspection
on its type enum. isSharedMap() was previously implemented with a vtable
slot, and we had no way of asking if an ArrayData was a NameValueTable.
Our ext_hhvm generated code is casting TypedValue* to Value*
on the assumption that the offset of TypedValue::m_data is 0.
Fix this assumption, and also while in the same code, replace
some (t == KindOfString || t == KindOfStaticString) with
IS_STATIC_STRING(t), which does a single bit test instead of
two comparisons.
A few simple refactorings and optimizations for array_data. Perflab results (CPU time %) are on the desired side of noise: 22/23 show reduced times, 12/23 green, no red, best -2.2%, worst +0.3%.
This mimics what TranslatorX64 does in translateSetMArray,
but it does it with fewer helpers and (often) fewer instructions in
translated code. I also found a bug in both jits and the interpreter
when dealing with arrays that hold refs to themselves. The new test
case exercises the fix, which involved a bit of refactoring of the
refcounting logic.
Enabling VectorTranslator while punting to tx64 is no longer a
regression so I removed the punt in emit().
Replace "collection" with "collections" in various file names since
we typically use the plural form in conversation and documentation.
Add set() and removeAt() methods needed for collection interfaces. Also
add the KeyedIterable and KeyedIterator interfaces.
Add __construct() methods for collections
Remove HPHPc-only inconsistencies from the inconsistencies doc. Also,
clarify the wording to make it clear we are comparing HipHop VM with the
Zend PHP engine.
Access to TypedValue.m_aux must now be via TypedValueAux. For now,
TypedValueAux is an empty subclass with accessors to m_aux, which is
now private. Once RefData.m_tv is moved out from under RefData._count,
we can move TypedValue.m_aux to TypedValueAux.
Removed unnecessary initialization of m_aux.u_hash from c_Vector.
This diff removes initializing stores to TypedValue._count, renames
_count to m_aux, and makes m_aux a union with members typed
and named according to their specialized uses. The few remaining
uses of that field for random tweaks are more obvious and easy to
grep for.
TypedValue no longer extends Value, (allowing m_data to move to a
different offset in the future), and Variant now extends TypedValue,
so we only have to maintain one definition.
HphpArray now explicitly uses TypedValue.m_pad instead of overlapping
TypedValue with an anonymous struct, again so we don't have to maintain
another structure to match TypedValue's layout.
The JIT's were using offsetof(TypedValue, _count) all over the place
for access to String/Array/Object/RefData::_count. Instead, use
FAST_REFCOUNT_OFFSET.
SharedMap was the last dependency on ZendArray. For its localCache,
use a TypedValue[] array indexed by SharedVariant.getIndex(), and
for escalate(mutableIteration), escalate to an HphpArray instead of
a ZendArray.
We only have recursion if what we're currently looking at is the same as
one of its parents in the nested arrays. We don't need to keep track of
everything that's been seen, only the elements seen in a path down to that
element.
Some StringData* helpers were forwarding to CStrRef
methods - which now forward to StringData* methods. Just
call the StringData* methods directly
Other methods were using stack allocated StringData's.
Thats pointlessly risky for slow-path code. Use a String
instead.
g++-4.7.1 treats "FOO"bar as a c++-11 literal operator, even
if bar is a macro with an expansion such as "BAR" - so add a space
after the quote (this seems like a bug, and I fixed a bunch of these
a while ago, but we just added a slew of PRI*64 macros which break
under 4.7.1).
Also, it warned that "explicit by-copy capture of 'this' redundant"
for a lambda declared [=, this] - so I removed the this.
We also needed more than the 60 levels of template expansion that was
allowed by the makefile.
This diff refactors some of the VM's logic for iterators (with a focus on
mutable iteration), delivering several improvements:
1) MIterCtx was renamed to MArrayIter, and the m_key and m_val fields
were eliminated.
2) Eliminated the need for MArrayIter to dynamically allocate a
MutableArrayIter object, and removed other layers of indirection as
well.
3) Reduced the size of HPHP::VM::Iter from 64 bytes down to 32 bytes.
4) Removed the "if (siPastEnd())" check when adding a new element to an
HphpArray or a ZendArray.
5) Moved all of the iterator logic into a single .cpp file.
This diff reworks FullPos's to point to current element instead of pointing
to the next element. It also splits up the IterFree instruction into two
instructions (IterFree and MIterFree). These changes allowed various logic
to be simplified and data structures to be reduced in size. There is
definitely more opportunity for refactoring, but I know the JIT helpers for
iteration have been carefully tuned and so I'll leave further refactoring
for future diffs.
Finally, I spent a little time cleaning up the bytecode spec a bit, mostly
with respect to iteration.
Per @mwilliams' suggestion, this is the first stage in a staggered approach to replacing int64 with int64_t. More precisely I inserted "typedef ::int64_t int64;" in util/base.h and dealt with the consequences.
This change is mostly for FB internal organizational reasons.
Building is not effected beyond the fact that the target now
lands in hphp/hhvm/hhvm rather than src/hhvm/hhvm.
Mark Williams