Bug 151301 - Remove unnecessary PLATFORM(EFL) macro in globalMemoryStatistics()
Summary: Remove unnecessary PLATFORM(EFL) macro in globalMemoryStatistics()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-15 22:35 PST by Gyuyoung Kim
Modified: 2015-11-19 04:59 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.53 KB, patch)
2015-11-15 22:36 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (1.67 KB, patch)
2015-11-18 02:55 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2015-11-15 22:35:52 PST
EXECUTABLE_ALLOCATOR_FIXED is enabled on EFL port. So we don't need to keep PLATFORM(EFL) macro anymore.
Comment 1 Gyuyoung Kim 2015-11-15 22:36:43 PST
Created attachment 265570 [details]
Patch
Comment 2 Csaba Osztrogonác 2015-11-16 03:02:21 PST
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.
Comment 3 Gyuyoung Kim 2015-11-16 18:35:23 PST
(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();
}
Comment 4 Gyuyoung Kim 2015-11-17 18:12:28 PST
(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 ?
Comment 5 Csaba Osztrogonác 2015-11-18 02:30:33 PST
(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;
Comment 6 Gyuyoung Kim 2015-11-18 02:55:24 PST
Created attachment 265739 [details]
Patch
Comment 7 Gyuyoung Kim 2015-11-18 15:13:43 PST
Ossy ping ?
Comment 8 WebKit Commit Bot 2015-11-19 03:58:43 PST
Comment on attachment 265739 [details]
Patch

Clearing flags on attachment: 265739

Committed r192624: <http://trac.webkit.org/changeset/192624>
Comment 9 WebKit Commit Bot 2015-11-19 03:58:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 2015-11-19 04:20:51 PST
(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.
Comment 11 Csaba Osztrogonác 2015-11-19 04:59:02 PST
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 .