The cache had a generation count, removing this check breaks eo suite.
EO is designed to tolerate init/shutdown cycles (as long as
dlopen/dlclose isn't involved).
Note: the ugly goto are useless as GCC -O2 understands EINA_(UN)LIKELY
and deals with it as expected (just look at the asm produced).
See 34d9f20706
This makes eo print a WRN message in case a function is called on NULL.
efl_del is an exception to this rule (implemented in a hackish way, I
admit). I don't know any language or object model where using a null
object doesn't result in an exception or crash, except EO. In any case,
calls to null are invalid.
The next commits will resolve most warnings for EFL.
Ref T6326
Summary:
ecore_evas: remove debug
eina: unregister log level when done with
Fixes a constant memory leak.
eina: introduce EINA_HOT and EINA_COLD
These attributes respectivelly expand to __attribute__ ((hot)) and
__attribute__ ((cold)) when available. They allow to mark functions are
being hot/cold (frequently used or not) as well as to qualify labels
within a function (likely/unlikely branches).
eo: speed-up generated calls by removing call cache
The call cache needed to by thread-local, to avoid concurrency issues.
Problem with TLS is that is adds an extra overhead, which appears to be
greater than the optimization the cache provides.
Op is naturally atomic, because it is an unsigned integer. As such, it
cannot be tempered with while another thread is reading it. When
entering the generated function, the first operation done is reading
'op'. If we have concurrency, we will have access sequences returning
either EFL_NOOP or a VALID op, because 'op' is not set until the very
end of the function, when everything has been computed. As such, we now
use the 'op' atomic integer to instore a lock-free/wait-free mechanism,
which allows to drop the TLS nature of the cache, speeding up the access
to the cache, and therefore making functions execute faster.
We don't test anymore the generation count. This can be put as a
limitation. If means that if you call efl_object_shutdown() and
re-initialize it later with different data, opcodes will be invalid.
I am not sure there is any usecase for this to ever happen.
We could move all the caches in a dedicated section, that can be
overwritten after a call to efl_object_shutdown(), but I am not sure it
will be very portable.
Benchmark: mean over 3 executions of
ELM_TEST_AUTOBOUNCE=100 time elementary_test -to genlist
```
BEFORE AFTER
------------------------------------------------------------
time (ns) 11114111647.0 9147676220.0
frames 2872.3333333333335 2904.6666666666665
time per frame (ns) 3869364.6666666665 3149535.3333333335
user time (s) 11.096666666666666 9.22
cpu (%) 22.666666666666668 18.333333333333332
```
Ref T6580
Reviewers: raster, cedric
Subscribers: cedric, jpeg
Maniphest Tasks: T6580
Differential Revision: https://phab.enlightenment.org/D5738
we can't sensibly use things like massif to track memory if we bypass
itr with mmaping -1 fd anonymous memory... so if built with valgrind
support and running under valgrind, use malloc/calloc and free so
these tools actually do something useful for these bits of memory.
Triggered after (almost) complete destruction of the object.
Not called "deleted" because the other event is already "del".
I don't like "destruct" much but this follows the terminology of
"constructor" / "destructor".
@feature
This reverts commit f910ba248e.
The scheduler is meant to be used only in C, not by bindings so there isn't really
a use for it in the loop class. Now this patch was triggered due to complexity in
using future/promise, so will do a follow up patch to improve that.
Efl.Future is an EO object which means even cancelling Efl.Future
objects requires EO. So this should be done before shutting down EO,
otherwise everything fails badly.
I believe Efl.Future is going to disappear soon, but the problem will
remain: if any promise/future uses EO or anything else outside of Eina
(so, basically anything) then it needs to be canceled before shutting
down the above layers. This is the same situation as with ecore events,
for which we've introduced ecore_event_type_flush.
Ping @cedric
in some cases construction can fail badly and this causes a crash...
eoid is NULL going in and this causes a cascade of failure. survive
and be more robust.
This API is meant to be used by parts only, and by bindings dealing with
part objects. This patch fixes make check which got broken in the after
the previous one (cxx).
luajit wants to check the upper 17 bits of pointers and assume they
are all 0 if on a 64bit architecture. it will panic and barf if they
are used. we have been using them in our eoid's for a long time.
my take on this is that this is unportable. assuming how many bits are
or are not valid in an opaque pointer (void *) that is simply passed
to you and you cannot dereference or use (no size information even for
what amount of data it points to etc.), is just wrong. it's not
portable and it's trying to be too smart and creating such issues. my
take is that luajit needs a fix for this in the longer term. but for
now let's have a 47 bit mode and go with that. it does mean i have to
drop our generation counter to 10 bits on 64bit (from 27 bits) which
increases likelihood of eoid re-use being falsely detected as valid
(before on 64bit it was 1 in 130 million or so chance, with this 47
bit change it's 1 in 1000. but to be fair on 32bit it's 7 bits for gen
count so 1 in 127 ... so still more than 10x "safer" than on 32bit...
but still...). the relevant check in luajit is:
(((uint64_t)(p) >> 47) ? (lj_err_msg(L, LJ_ERR_BADLU), NULL) : (p))
it ONLY does this on 64bit. on 32bit pointers are not checked for
validity at all.
as an aside, armv8.2 seemingly will bring 52bit addresses so luajit is
going to fall over flat on a newer rev of armv8. it may be x86 also
uses more bits. last i knew it was 48bits so the 47 bit check luajit
does i think is even wrong for x86-64. to be detailed i read:
amd64 arch == 48 bits (so luajit is wrong). even better In addition,
the AMD specification requires that the most significant 16 bits of
any virtual address, bits 48 through 63, must be copies of bit 47 (in
a manner akin to sign extension). so if the upper bit of 48 is set
THEN all the 16 upper bits must be 1... breaking luajit, even if it
were 47bit and this rule applied. I read the architecture allows for
up to 52bits of actual addresses so architecture-wise this is even
wrong...
So I smell a core bug here in luajit. Certainly in the number of bits
it insists must be 0 (the upper 17 bits where on amd64/x86-64 it
should be the upper 16 bits... and even then these may NOT be 0 if bit
47 (the upper bit of the lower 48 is 1).... so the whole check is
invalid... :(
at least the above is at a theoretical level. i believe that the
addresses divide the 48 bits into 2 chunks (thus 47)... but at the
PHYSICAL level with no mmu and virtual memory. arm64 has this:
https://www.kernel.org/doc/Documentation/arm64/memory.txt
note in all cases the 2nd chunk of memory has at leats some upper bits
of physical addresses beign 1 ... which makes luajit invalid tyo use
without virtual memory remapping these away from high bits.
@fix
It would be pretty stupid to create an object that dies before it even
gets created. Auto-unref is for parts and should probably not be used
anywhere else :)
the freeq can't allow things to access the content of the pointer
because the content canbe written over by canary handling qhen
submitted to the freeq. clear it inline then just submit the final
pointer to the freeq.
@fix
Before screaming in horror (C++...) here's why we may need this:
Efl.Part.part API returns an object that is by definition valid for a
single function call only. Enforcing this in practice is actually quite
hard as all implementation functions must manually take care of the
life-cycle. This is a lot of code in many places and a lot of
opportunities to forget to properly handle that life-cycle. Also, this
means any invalid function call on a part will leak an object.
This API absolutely must remain either "internal" or "beta" and
definitely not become abused by applications. On top of that such an API
can cause great trouble for bindings like C++. As a consequence, only
specially crafted APIs like efl_part() should return an object marked as
auto_unref.
Alternatively this API could be defined in Eo.h or some other
Eo_Internal.h. I placed it in efl_object.eo because it's much more
convenient :) (read: I'm lazy)
****
Performance notes:
Tested with clang & gcc (with -O2), I had a look at the output of perf
top, in particular the asm view. I used eo_bench in a loop. My
conclusions are:
- EINA_LIKELY/UNLIKELY actually works. The jump statement varies
according to the expectation. I highly doubt all those ugly goto in
eo.c / Eo.h are even useful.
- The impact of auto_unref on a call_resolve is so small it doesn't even
appear in the trace. It is significant inside call_end, though
(obviously, that function is just a few lines long). That function
accounts for ~1% to ~4% of all CPU time. The impact of auto_unref in
call_end is ~4% of the function time. This means ~0.16% of all CPU
time (worst measured case). _efl_object_op_api_id_get simply doesn't
show up because of caching, so the extra check there is negligible.
PS: I also tested EINA_LIKELY/UNLIKELY by compiling with -O2 and looking
at the output with objdump. The flag is well respected, and the jump
instructions are what you would expect (no jump for LIKELY and jump for
UNLIKELY). Conclusion: The goto's in eo.c only make the code harder to
read...
Simply pass in the strbuf and don't expect the callee to own it. This
makes things simpler and safer (it'll crash only if the callee frees
said strbuf, and shouldn't leak). efl_ebug_name is new in the upcoming
release, EFL 1.21.
Realised this after talking with Amitesh. Thanks.
See 999dbd9764
And c4769ff898
there can be cases where the call to such a function is never going to
be directed to the actual implementation. In those cases the FallackCall
will be called.
This allows deleting an object by simply calling efl_unref() on it, even
if there is a parent. Normally the parent is in charge, and you can
request deletion by calling efl_del() or efl_parent_set(NULL).
But in some rare cases, you want to give ownership of an object (@owned)
and still give a parent to that object. efl_unref() should be used (and
would be used by bindings when going out of scope or on garbage
collection), which would then print an error message. This API allows
the specific behaviour.
@feature
export EO_LIFECYCLE_BACKTRACE=0
This means that the lifecycle can be tracked but we don't care about the
backtraces. Related to some recent eo_debug patches.
If an object still has a parent inside the del intercept, we shouldn't
reset the "parent_sunk" flag. This would indeed break logic as
parent_sunk == false implies that a parent was never set, which means
the parent must be null. Right now this case isn't used but it can be
imagined with caches of evas objects (they should remain parented to
their evas).
@fix
efl_data_scope_safe_get() is often used to assert that an object is of a
certain type, but assuming it may be NULL or not of the required type.
This means that encountering an invalid type is an error handled by the
caller (we return NULL), and shouldn't print out extra ERR() logs.
This fixes issues with E run under eo_debug. E's code was safe as it's
using evas_object_smart_data_get() and verifies whether the returned
value is NULL (which is expected for a rectangle, for instance).
@fix
This is done via the syntax (not documented yet):
EO_LIFECYCLE_DEBUG=N:<class names>
Where N is:
1. new/free
2. new/free/ref/unref
3. new/free/ref/unref/reuse
@feature
It's not enough to check the init generation count, if the cache data is
not reset properly.
This patch adds safety in two ways:
1. Actually reset the op call cache when we detect a new generation
2. Verify that we don't return NULL function pointer
The 1st point is absolutely necessary, 2nd is optional (extra cost of
one if()).
@fix
It's a complex struct but defined in EO as a simple struct. ABI-wise
it's equivalent to Eina_Rectangle. Some macros that use Eina_Rectangle
also work on Eina_Rect out of the box, most of the code dealing with
x,y,w,h will require no modifications either.
But Eina_Rect provides direct access to a size or position 2d component,
as well as the usual x,y,w,h. The field "rect" is provided as a
convenience for code dealing with both Eina_Rectangle and Eina_Rect. We
may or may not require it.
Note: Size2D could use unsigned values but I have spotted a few places
in the code that actually use -1 to indicate invalid size (as opposed to
0x0).
@feature
Cedric BAIL
Vincent Torri
Jean-Philippe Andre
Jean Guyomarc'h
Carsten Haitzler
Andy Williams
Marcel Hollerbach
Daniel Kolesa
Lauro Moura
Marcel Hollerbach