Step two. Leaving the Variant::same methods as deleted for
now because I'm worried about the implicit conversion operators to
other types that support same.
They aren't used anymore, except to implement isVectorData(),
which we can make pure-virtual and implement more efficiently
in each subclass of ArrayData.
I need to walk a live eval stack, handling FPI regions and
generators, and duplicating that logic in yet another place seems
wrong. This makes the Stack::toString stuff use a reusable one---at
some point I'll see if the unwinder can use it as well, but it works
also for my use case. Also, Stack::toString was broken for mutable
array iterators---fixes that. And use a union now for struct Iter.
And don't include bytecode.h from func.h.
nvSet() only casts the value from TypedValue* to const Variant&; do it
at callsites. Inlined array_setm_ik1_v0() and array_setm_s0k1_v0() into
their only remaining callsites in translator-runtime.cpp.
The in-situ allocation case can be easily detected by checking m_data == m_inline_data.slots. Therefore there's no need for two distinct flags to track current and future allocation strategy.
Alerts users that comparators should be in the
style of functions passed to e.g. C qsort (i.e. returning
-1, 0, or 1) rather than STL-style boolean comparators (i.e.
true for less-than).
The codebase had several namespace-level static data definitions and function definitions. Using namespace-level "static" in a .h file is near-always a bad idea, as follows:
- for simple types, static is implied. Example: "const int x = 42;" is the same as "static const int x = 42;"
- for aggregate types, static linkage implies that a copy of the aggregate will appear in every compilation unit that includes the header.
- for functions, static means the function will have a separate body generated in each compilation unit including the header.
- in several places functions were defined 'static inline', which is just as bad. 'inline' is just a hint which means the function may end up having a body (and actually more due to 'static').
True, gnu's linker has means to remove duplicate definition, but that's not always guaranteed or possible (think e.g. static functions that define static data inside, ouch). So static is useless at best and pernicious at worst. We should never, ever use static at namespace level in headers. I will create a bootcamp task for a lint rule.
I expected the performance to be neutral after the change, but in fact there's a significant drop in instruction count and therefore a measurable reduction in CPU time: https://our.intern.facebook.com/intern/perflab/details.php?eq_id=431903
After Variant::Variant(NullInit) was made explicit, passing
Variant::nullInit to a function expecting CVarRef, or Variant
would actually call Variant(0), with ensuing hilarity.
Make NullInit an enum class to prevent such issues.
Make internal iterator API return a TypedValue* instead of Variant. This
makes it possible to use the same API to update the value in-place
(except for Set, which returns const TypedValue*) and provide optimized
iterator that avoids unnecessary ref counts.
Functions like array_filter and array_map need "withRef" semantics,
and while it can be simulated in php via copy-on-write, doing so
is very inefficient.
This adds
SetWithRefLM
SetWithRefRM
- similar to SetM but binds the value if it was a reference. L reads a local, R reads a return value
WIterInit
WIterInitK
WIterNext
WIterNextK
- essentially the same as the corresponding opcodes without W, but the value local is set by reference if the array element was a reference.
emitAddNewElemC didn't properly implement the specified
behavior for this opcode. It assumed topC(1) was always an non-static
array with refcount 1 or 0. This changes to interp the case when it's
not an array, and make the helper support static arrays. (If it
matters for perf, we should engineer the bytecode spec to require this
as well, since the point of this opcode is initializing array literals
that can't be done as static arrays.) In practice I think this is
only working because it always comes after a NewArray with a capacity
hint != 1, which means it's a newly allocated non-static array.
Ignoring the capacity hint would've broken it.
In runtime code, use StaticString for literals or String for computed
char strings. In test code, I justed used String("literal") to keep
the conversion simpler. I think tests are ultimately easier to maintain
without the StaticString treatment.
ArrayData gets on a diet, down to 32 bytes
devirtualized destructor call in HphpArray::release()
aggressively inlined constructors for ArrayData (do away with defaulted arguments)
eliminate a dead memset() for the hashtable in HphpArray(uint size, const TypedValue* values)
I've done this refactoring without efficiency in mind - it's one of the steps toward more aggressive in-situ allocation. Yet to my surprise perflab rewarded this simple refactoring with a whopping decrease in I-TLB misses. I myself am wondering what has caused this. Anyhow let's run perflab once more and get this baby on the road.
I've run a bunch of experiments on HphpArray speed and am still yet to have a major breakthrough. However, there are a few clear small winners that I'm submitting with this diff. CPU instructions, CPU loads, and CPU stores are all as green as the Eternal Hunting Grounds. Speed is drowned in noise but I sususpect will be measurable on these changes combined.
I'm committing my work on HphpArray in bite-sized pieces for easier review. This piece replaces calls to NEW with a factory method. There are many problems with operator new, starting with the fact that the allocator cannot communicate properly with the constructor.
The newly introduced factory method should return ArrayData but that causes many issues right now, so I left that step to a future diff.
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.
Jordan DeLong