The Func*'s in the native func unit are all persistent, which means
that we can strip them, and skip the Unit::merge call on every
request. But we didnt set the flag to compact the unit if the only
things that needed stripping were Func*'s. Fixed that, and also fixed
it so all the builtins are marked persistent even in non-repo-auth
mode (funcs are not if rename function is enabled, because we implement
fb_rename_function by swapping target cache entries).
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.
We end up with uninit nulls in the repo for unit constants.
In RepoAuthoritative mode, we will use m_type == KindOfUninit during
constant lookup to decide whether to cast TypedValue::m_data.pref to a
ConstantInfo*, and then call a function pointer that it contains. (We
should probably also clean that up to not cast RefData*'s to
ConstantInfo*'s, but I left that out of this diff.)
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
This func took 2 params and ignored the second one. While I was there I made the functions call eachother. Hopefully inlining is smart enough to let me clean this code.
I got a bit carried away, but I think this is better.
This cleans up the code a lot, and takes it out of various hot
paths. It will impact perf for requests where intercepts are used,
but no longer penalizes requests that don't use intercepts.
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.
Add a lot of comments to the debugger based on my current understanding of it. These may change in the future as we learn more, but they're helpful right now.
Also moved a few small things around in the code to clarify their purpose or scope. I.e., making a few things private, renaming a few functions, etc. No real logic changes, though. Also minor dead code removal. Also a few lint errors.
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.
While investigating why gcc-4.7.1 is slower than gcc-4.6.2,
I noticed that under 4.6.2 the tbb code was being inlined into
Unit::GetNamedEntity, but under 4.7.1 it wasn't. Perf showed that
the inclusive time for GetNamedEntity was about .4% higher under
gcc-4.7.1.
Meanwhile, after various code changes, gcc-4.6.2 is no longer
inlining the tbb code either. Playing with the inlining options,
I wasnt able to get either version of gcc to inline it anymore,
but using __attribute__((__flatten__)) works for both. But that
also causes a bunch of other, slow-path code to be inlined, so
split it into a hot and a cold function, and flatten the hot
function.
Perf shows about a .2% win for Unit::GetNamedEntiry under gcc-4.6.2
Only in RepoAuthoritative mode, where units can't be
deallocated. I have a semi-reproducable bug where a unit's m_bc
region is getting corrupted (only occasionally in perflab).
Presumably moving it out of the malloc'd heap will make the bug
corrupt something else, so this probably doesn't really help much, but
it seemed like having a separate region for some cold, read-only,
process-lifetime metadata might make sense.
Adds runtime support for non-class typehints. Typedefs are
introduced using type statements, and autoloaded via a new autoload
map entry. Shapes are parsed but the structure is currently thrown
away and treated as arrays at runtime. This extends the NamedEntity
structure to sometimes cache 'NameDefs', which are either Typedef*'s
or Class*'s. VerifyParamType now has to check for typedefs if an
object fails a class check, or when checking non-Object types against
a non-primitive type name that isn't a class.
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.
In Zend 5.3 they decided that closures should inherit the ##$this## from the containing scope. This brings us close to paraity with them. The remaining thing is to make
function() use ($this) {}
fatal after we purge it from WWW.
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.
I found some byte-wide padding in Unit and UnitEmitter that we can
use to store the mergeOnly flag, instead of m_returnValue's unused
_count field. This is one step towards eliminating TV._count.
This turns the output into three columns: tx64 asm, hhir asm,
then pretty-printed hhir with asm inline. This should help us figure
out what the ir is doing wrong when it makes bigger code. I also
compacted the output of Trace::print a bit and changed it to print
unlikely blocks at the very end.
I found 4 separate problems, each of which broke this:
- A hoistable class didnt generate bytecode, so the line number
information was associated with whatever line the /next/
bytecode corresponded to. Often there was no next bytecode,
so we got the line number of the end of the file.
- If there /was/ a bytecode for the defClass, the line number
recorded for it was that of the pseudoMain, not the class
itself; so again, it would be reported as the end of the file.
- The parser records two sets of line numbers for a class; its
start, and end. The end line gets associated with the bytecode.
- There were optimized paths where we didnt setup an ActRec
at all, resulting in either reporting the file/line of the
require (or often a require much higher up the stack), or
not reporting a file/line at all.
So now, we generate a nop if we would have skipped the defCls.
We override the normal line info, and set both the start and end
lines to the start. And we wrap all errors from defClass in a
temporary frame to ensure the line info is seen.
This changed the output for several of our tests. Spot checking
a couple showed they now report the same line number as zend.
Try to scoop up some more instructions in the IR. In a
sequence like:
BoringInstrA
BoringInstrB
PuntInstr
BoringInstrC
we used to punt the entire tracelet to interp. Now we do:
hhir:
BoringInstrA
BoringInstrB
interp:
PuntInstr
BoringInstrC
Rescuing BoringInstrC from PuntInstr is future work.
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.
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