Remove unnecessary PLATFORM(EFL) macro in globalMemoryStatistics()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gyuyoung Kim]

EXECUTABLE_ALLOCATOR_FIXED is enabled on EFL port. So we don't need to keep PLATFORM(EFL) macro anymore.

[Gyuyoung Kim]

Created attachment 265570 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 265570 [details]
Patch

I agree to remove (PLATFORM(EFL) && ENABLE(JIT)), but I think we could remove the whole #else case.

I don't understand how ExecutableAllocator::committedByteCount()is related to ENABLE(EXECUTABLE_ALLOCATOR_FIXED) guard.

ENABLE(EXECUTABLE_ALLOCATOR_FIXED) is enabled only id CPU(X86_64) || PLATFORM(IOS) || CPU(ARM64),
so it isn't enabled at least on x86, ARM Linux.

But why do we have any guard for using ExecutableAllocator::committedByteCount() ?
It is implemented if ENABLE(EXECUTABLE_ALLOCATOR_FIXED) is true or false.

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 265570 [details]
> Patch
> 
> I agree to remove (PLATFORM(EFL) && ENABLE(JIT)), but I think we could
> remove the whole #else case.
> 
> I don't understand how ExecutableAllocator::committedByteCount()is related
> to ENABLE(EXECUTABLE_ALLOCATOR_FIXED) guard.
> 
> ENABLE(EXECUTABLE_ALLOCATOR_FIXED) is enabled only id CPU(X86_64) ||
> PLATFORM(IOS) || CPU(ARM64),
> so it isn't enabled at least on x86, ARM Linux.
> 
> But why do we have any guard for using
> ExecutableAllocator::committedByteCount() ?
> It is implemented if ENABLE(EXECUTABLE_ALLOCATOR_FIXED) is true or false.


committedByteCount is supported both by ENABLE_EXECUTABLE_ALLOCATOR_DEMAND and ENABLE_EXECUTABLE_ALLOCATOR_FIXED. So it looks almost ports use one of both at least. I wonder what JS guys think about it.

size_t ExecutableAllocator::committedByteCount()
{
    return DemandExecutableAllocator::bytesCommittedByAllocactors();
}

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 265570 [details]
> Patch
> 
> I agree to remove (PLATFORM(EFL) && ENABLE(JIT)), but I think we could
> remove the whole #else case.
> 
> I don't understand how ExecutableAllocator::committedByteCount()is related
> to ENABLE(EXECUTABLE_ALLOCATOR_FIXED) guard.
> 
> ENABLE(EXECUTABLE_ALLOCATOR_FIXED) is enabled only id CPU(X86_64) ||
> PLATFORM(IOS) || CPU(ARM64),
> so it isn't enabled at least on x86, ARM Linux.
> 
> But why do we have any guard for using
> ExecutableAllocator::committedByteCount() ?
> It is implemented if ENABLE(EXECUTABLE_ALLOCATOR_FIXED) is true or false.

Ossy, do you think we should land this patch with removing #else statement ?

[Csaba Osztrogonác]

(In reply to comment #4)
> Ossy, do you think we should land this patch with removing #else statement ?

I meant the following change:

- #if ENABLE(EXECUTABLE_ALLOCATOR_FIXED) || (PLATFORM(EFL) && ENABLE(JIT))
    stats.JITBytes = ExecutableAllocator::committedByteCount();
- #else
-    stats.JITBytes = 0;

[Gyuyoung Kim]

Created attachment 265739 [details]
Patch

[Gyuyoung Kim]

Ossy ping ?

[WebKit Commit Bot]

Comment on attachment 265739 [details]
Patch

Clearing flags on attachment: 265739

Committed r192624: <http://trac.webkit.org/changeset/192624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #8)
> Comment on attachment 265739 [details]
> Patch
> 
> Clearing flags on attachment: 265739
> 
> Committed r192624: <http://trac.webkit.org/changeset/192624>

Oh, it broke the CLOOP build, I'll fix it immediately.

/Volumes/Data/slave/yosemite-cloop-debug/build/Source/JavaScriptCore/runtime/MemoryStatistics.cpp:40:22: error: use of undeclared identifier 'ExecutableAllocator'
    stats.JITBytes = ExecutableAllocator::committedByteCount();
                     ^
1 error generated.

[Csaba Osztrogonác]

Platform.h:
------------
...
#if ENABLE(ASSEMBLER)
#if CPU(X86_64) || PLATFORM(IOS) || CPU(ARM64)
#define ENABLE_EXECUTABLE_ALLOCATOR_FIXED 1
#else
#define ENABLE_EXECUTABLE_ALLOCATOR_DEMAND 1
#endif
#endif
...

So ENABLE(EXECUTABLE_ALLOCATOR_FIXED) || ENABLE(EXECUTABLE_ALLOCATOR_DEMAND)
is true only if ENABLE(ASSEMBLER) is true. 

Fix landed in http://trac.webkit.org/changeset/192626 .