Bug 91214 – [EFL][WK2] Add WebMemorySampler feature.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[JungJik Lee]

WebMemorySampler is for logging WebProcess and MiniBrowser's memory usage in real time.
The log file will be created in /tmp path. and the name of process will be /tmp/WebProcessXXXX and /tmp/MiniBrowserXXXX.
This log includes javascript memory usage, fast malloc memory usage and basic process memory usage(RSS/Shared/Library/Stack).
Basic memory info of the process is taken from /proc/<process_id>/statm.

[JungJik Lee]

Created attachment 153226 [details]
proposal patch

[WebKit Review Bot]

Attachment 153226 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1
Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:56:  MAX_BUFFER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:57:  MAX_PROCESS_PATH is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JungJik Lee]

Created attachment 153233 [details]
fix style error.

[JungJik Lee]

Created attachment 153474 [details]
Patch

[Dominik Röttsches (drott)]

Comment on attachment 153474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153474&action=review

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:127
> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const

This is overlapping with Bug 91274. How about making the rest of this patch available in window.internals, too?

[JungJik Lee]

Comment on attachment 153474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153474&action=review

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:127
>> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const
> 
> This is overlapping with Bug 91274. How about making the rest of this patch available in window.internals, too?

Okay, no problem. I will make a new patch in window.internals.

[Dominik Röttsches (drott)]

(In reply to comment #6)
> (From update of attachment 153474 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153474&action=review
> 
> >> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:127
> >> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const
> > 
> > This is overlapping with Bug 91274. How about making the rest of this patch available in window.internals, too?
> 
> Okay, no problem. I will make a new patch in window.internals.

The other patch hasn't received any review - so I am not sure yet, whether that approach is accepted. It would be just good to have a unified approach for both cases.

[JungJik Lee]

Comment on attachment 153474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153474&action=review

>>>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:127
>>>> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const
>>> 
>>> This is overlapping with Bug 91274. How about making the rest of this patch available in window.internals, too?
>> 
>> Okay, no problem. I will make a new patch in window.internals.
> 
> The other patch hasn't received any review - so I am not sure yet, whether that approach is accepted. It would be just good to have a unified approach for both cases.

Well, I agree that it is good to have a unified approach. but I think this patch is a little different from bug 91274. We should add js to test site to profile memory usage and if we do, the result would be different.

[Raphael Kubo da Costa (:rakuco)]

Perhaps it's time to revisit this now that bug 91274 has been fixed?

[JungJik Lee]

(In reply to comment #9)
> Perhaps it's time to revisit this now that bug 91274 has been fixed?

Yes I saw the 91274 patch and it is merged. so how do you think? Do you think this patch still has meaning? My opinion is that those have a similar function but different purpose.

[Gyuyoung Kim]

(In reply to comment #10)
> Yes I saw the 91274 patch and it is merged. so how do you think? Do you think this patch still has meaning? My opinion is that those have a similar function but different purpose.

I think you need to explain why this patch is still valid. It seems patch of Bug 91274 is enough for our port as well.

[JungJik Lee]

(In reply to comment #11)
> (In reply to comment #10)
> > Yes I saw the 91274 patch and it is merged. so how do you think? Do you think this patch still has meaning? My opinion is that those have a similar function but different purpose.
> 
> I think you need to explain why this patch is still valid. It seems patch of Bug 91274 is enough for our port as well.

If we try to get memory usage by using Bug 91274, we should add some js code inside the site but we usually couldn't write js inside the page because it's not our test page. it seems like for web-developer. But this patch is able to measure the memory usage without handling user-page. And gives more detail info about memory usage in real time.

[JungJik Lee]

Created attachment 158812 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 158812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158812&action=review

First of all, I'm not sure if we can support memory usage functionality as APIs. Can't we support this functionality when we set environment variable ? In addition, I think this patch needs some loves. So. I set r-. In addition,

> Source/JavaScriptCore/PlatformEfl.cmake:20
> +IF (ENABLE_MEMORY_SAMPLER)

ENABLE_XXX macros were removed to avoid duplicated #ifdef guard. However, though MemoryStatics.cpp file is not guarded by #if ENABLE(XXX), I would like to recommend to add the MemoryStatistics.cpp to existing JavaScriptCore_SOURCES.

> Source/WTF/wtf/Platform.h:1152
> +#if PLATFORM(EFL)

We don't need to add this macro to here.

> Source/WebKit2/PlatformEfl.cmake:26
> +    Shared/efl/WebMemorySamplerEfl.cpp

Please move this line to below block.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:24
> + *

Remove unneeded line.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:55
> +} ApplicationMemoryStats;

Duplicated struct name declaration ?

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:63
> +        return String();

Please add a new line after return.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:66
> +    fscanf(file, "%s", buffer);

Please add a new line.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:238
> +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);

EFL port has add an empty line after EINA_SAFETY_ON_NULL_RETURN macro.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:245
> +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:139
> +* Start memory sampler.

Add a space before *.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:147
> +EAPI void ewk_context_memory_sampler_start(Ewk_Context* context, double timer_interval);

Nit : Wrong '*' operator place.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:150
> +* Stop memory sampler.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:154
> +EAPI void ewk_context_memory_sampler_stop(Ewk_Context* context);

Nit : Wrong '*' operator place.

> Source/cmake/OptionsEfl.cmake:55
> +SET(ENABLE_MEMORY_SAMPLER ON)

Please use WEBKIT_OPTION_DEFAULT_PORT_VALUE macro.

[JungJik Lee]

Created attachment 159015 [details]
Patch

[JungJik Lee]

Created attachment 160361 [details]
Patch

[JungJik Lee]

(In reply to comment #14)
> (From update of attachment 158812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158812&action=review
> 
> First of all, I'm not sure if we can support memory usage functionality as APIs. Can't we support this functionality when we set environment variable ? In addition, I think this patch needs some loves. So. I set r-. In addition,
> 
> > Source/JavaScriptCore/PlatformEfl.cmake:20
> > +IF (ENABLE_MEMORY_SAMPLER)
> 
> ENABLE_XXX macros were removed to avoid duplicated #ifdef guard. However, though MemoryStatics.cpp file is not guarded by #if ENABLE(XXX), I would like to recommend to add the MemoryStatistics.cpp to existing JavaScriptCore_SOURCES.
> 
> > Source/WTF/wtf/Platform.h:1152
> > +#if PLATFORM(EFL)
> 
> We don't need to add this macro to here.
> 
> > Source/WebKit2/PlatformEfl.cmake:26
> > +    Shared/efl/WebMemorySamplerEfl.cpp
> 
> Please move this line to below block.
> 
> > Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:24
> > + *
> 
> Remove unneeded line.
> 
> > Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:55
> > +} ApplicationMemoryStats;
> 
> Duplicated struct name declaration ?
> 
> > Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:63
> > +        return String();
> 
> Please add a new line after return.
> 
> > Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:66
> > +    fscanf(file, "%s", buffer);
> 
> Please add a new line.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:238
> > +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);
> 
> EFL port has add an empty line after EINA_SAFETY_ON_NULL_RETURN macro.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:245
> > +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);
> 
> ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:139
> > +* Start memory sampler.
> 
> Add a space before *.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:147
> > +EAPI void ewk_context_memory_sampler_start(Ewk_Context* context, double timer_interval);
> 
> Nit : Wrong '*' operator place.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:150
> > +* Stop memory sampler.
> 
> ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:154
> > +EAPI void ewk_context_memory_sampler_stop(Ewk_Context* context);
> 
> Nit : Wrong '*' operator place.
> 
> > Source/cmake/OptionsEfl.cmake:55
> > +SET(ENABLE_MEMORY_SAMPLER ON)
> 
> Please use WEBKIT_OPTION_DEFAULT_PORT_VALUE macro.

done everything! And remove the APIs and change the code by using environment value.

[Gyuyoung Kim]

Comment on attachment 160361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160361&action=review

I think this patch includes Bug 91274 functionality. So, it looks this patch is valuable for us. Any other opinions ?

Anyway, I think detail implementation should be reviewed further. I'm going to review it further.

> Source/JavaScriptCore/ChangeLog:3
> +        [EFL][CMAKE] Add WebMemorySampler option.

Add [EFL][WK2] prefix to bug title.

> Source/WTF/ChangeLog:3
> +        [EFL] Add WebMemorySampler option.

ditto.

> Source/WebKit2/ChangeLog:3
> +        [EFL] Add WebMemorySampler feature.

ditto.

> Source/JavaScriptCore/PlatformEfl.cmake:3
>      jit/ExecutableAllocator.cpp

Add a new line to distinguish different directories.

> Source/WebKit2/PlatformEfl.cmake:25
> +    Shared/WebMemorySampler.cpp

ditto.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:27
> +

Unneeded line.

> Source/cmake/OptionsEfl.cmake:82
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEMORY_SAMPLER ON)

You have to add this macro to Source/cmakeconfig.h.cmake as well.

[JungJik Lee]

Comment on attachment 160361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160361&action=review

>> Source/cmake/OptionsEfl.cmake:82
>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEMORY_SAMPLER ON)
> 
> You have to add this macro to Source/cmakeconfig.h.cmake as well.

This feature is not used in all ports. So I hope this code is limited to EFL port firstly. In addition if I add the macro to cmakeconfig.h.cmake, it shows compile warning.

[JungJik Lee]

Created attachment 160413 [details]
Patch

[Kangil Han]

Comment on attachment 160413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160413&action=review

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.

Do we need this?

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:81
> +            } else if (foundKeyName) {

When program comes in? Seems 'foundKeyName' always 'false' before if condition.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:92
> +    ApplicationMemoryStats applicationStats;

I think we can put this line inside if condition.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:109
> +    return applicationStats;

Then this would be 'return 0'

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:114
> +    String processName;

We would put this line inside if condition.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:116
> +    sprintf(processPath, "/proc/%d/status", getpid());

snprintf?

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:124
> +    return processName;

Then 'return String()'

[JungJik Lee]

Comment on attachment 160413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160413&action=review

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:2
>> + * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> 
> Do we need this?

I'm not sure. the template of this code is based on Apple code, so I leave it. Does this line cause any legal issue?

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:81
>> +            } else if (foundKeyName) {
> 
> When program comes in? Seems 'foundKeyName' always 'false' before if condition.

Yes, if the token has not colon, it will be the value.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:109
>> +    return applicationStats;
> 
> Then this would be 'return 0'

this function returns structure type.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:114
>> +    String processName;
> 
> We would put this line inside if condition.

I don't understand the reason.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:116
>> +    sprintf(processPath, "/proc/%d/status", getpid());
> 
> snprintf?

Good point, I'll fix it right now.

[JungJik Lee]

Created attachment 160467 [details]
Patch

[Benjamin Poulain]

Comment on attachment 160467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160467&action=review

> Source/WTF/wtf/Platform.h:1173
> +#if PLATFORM(EFL)
> +#define ENABLE_MEMORY_SAMPLER 1
> +#endif

Why do you even need that??

None of the code is in common WebKit.

[Benjamin Poulain]

Comment on attachment 160467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160467&action=review

> Source/WTF/wtf/Platform.h:1173
> +#if PLATFORM(EFL)
> +#define ENABLE_MEMORY_SAMPLER 1
> +#endif

Why do you even need that??

None of the code is in common WebKit.

[Kangil Han]

Comment on attachment 160413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160413&action=review

>>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:81
>>> +            } else if (foundKeyName) {
>> 
>> When program comes in? Seems 'foundKeyName' always 'false' before if condition.
> 
> Yes, if the token has not colon, it will be the value.

This looks 'if, else if' implementation.
Looks foundKeyName only being set 'true' inside 'if' loop.
How it would affect 'else if' loop in this function?

>>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:109
>>> +    return applicationStats;
>> 
>> Then this would be 'return 0'
> 
> this function returns structure type.

Yes, it is true. I overlooked it.

>>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:114
>>> +    String processName;
>> 
>> We would put this line inside if condition.
> 
> I don't understand the reason.

Search with 'String()' in webkit source code.
It's WebKit style. :-)

[Benjamin Poulain]

Comment on attachment 160467 [details]
Patch

I cq- because of my comments regarding Platform.h. This looks like a pure EFL feature.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 160467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160467&action=review

The current code is *very* JSC and Linux-specific. The JSC part cannot be easily solved, so you need to build all this only if JSC is being used instead of V8. For the Linux part, you need to either wrap your code within #if OS(LINUX) ifdefs or only build/call this at all if you are on Linux.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:34
> +#include <malloc.h>

This is *very* deprecated, and you don't seem to allocate memory with malloc(3) and friends at all.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:64
> +    memset(buffer, 0, maxBuffer);

Please include string.h for memset(3).

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:65
> +    fscanf(file, "%s", buffer);

And stdio.h for fscanf(3).

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:129
> +    size_t totalBytesInUse = 0, totalBytesCommitted = 0;

One variable per line, please.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:748
> +#if ENABLE(MEMORY_SAMPLER)
> +    static bool memorySampler = false;
> +    static const char environMemorySampler[] = "MEMORY_SAMPLER";
> +
> +    if (!memorySampler && getenv(environMemorySampler)) {
> +        WKRetainPtr<WKDoubleRef> interval(AdoptWK, WKDoubleCreate(0.0));
> +        WKContextStartMemorySampler(ewk_context_WKContext_get(context), interval.get());
> +        memorySampler = true;
> +    }
> +#endif

Why all this instead of using the existing infrastructure for enabling and starting the memory sampler?

[Gyuyoung Kim]

Comment on attachment 160467 [details]
Patch

r- because of many comments.

[Kangil Han]

Comment on attachment 160467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160467&action=review

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:152
> +    webKitMemoryStats.keys.append(String("Timestamp"));

As WebMemoryStatistics has standard pattern like below, consider create a function to reduce line numbers inside this function.

struct WebMemoryStatistics
{
    Vector<String> keys;
    Vector<size_t> values;
};

[JungJik Lee]

Created attachment 160672 [details]
Patch

[JungJik Lee]

Comment on attachment 160467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160467&action=review

>>> Source/WTF/wtf/Platform.h:1173
>>> +#endif
>> 
>> Why do you even need that??
>> 
>> None of the code is in common WebKit.
> 
> Why do you even need that??
> 
> None of the code is in common WebKit.

I moved it to PlatformEfl.cmake. thanks your comment.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:34
>> +#include <malloc.h>
> 
> This is *very* deprecated, and you don't seem to allocate memory with malloc(3) and friends at all.

Done, I removed it.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:64
>> +    memset(buffer, 0, maxBuffer);
> 
> Please include string.h for memset(3).

Done, I added string.h.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:65
>> +    fscanf(file, "%s", buffer);
> 
> And stdio.h for fscanf(3).

Done, I added stdio.h too.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:129
>> +    size_t totalBytesInUse = 0, totalBytesCommitted = 0;
> 
> One variable per line, please.

Done and thanks your comments.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:152
>> +    webKitMemoryStats.keys.append(String("Timestamp"));
> 
> As WebMemoryStatistics has standard pattern like below, consider create a function to reduce line numbers inside this function.
> 
> struct WebMemoryStatistics
> {
>     Vector<String> keys;
>     Vector<size_t> values;
> };

it's fixed by following your comment. thanks.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:748
>> +#endif
> 
> Why all this instead of using the existing infrastructure for enabling and starting the memory sampler?

could you let me know what the existing infrastructure is?

[Ryuan Choi]

Comment on attachment 160672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160672&action=review

> Source/WebKit2/PlatformEfl.cmake:189
> +    -DENABLE_MEMORY_SAMPLER=1

This is not needed if you want to add it in OptionsTizen.cmake.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:71
> +inline void appendMemoryStats(WebMemoryStatistics& webKitMemoryStats, String key, size_t value)

const String& key

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:106
> +    FILE* fMemoryStatus = fopen(processPath, "r");
> +    if (fMemoryStatus) {

empty line is preffered.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:118
> +#endif
> +    return applicationStats;

need empty line.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:134
> +#endif
> +    return String();

need empty line.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:748
> +#if ENABLE(MEMORY_SAMPLER)
> +    static bool memorySampler = false;
> +    static const char environMemorySampler[] = "MEMORY_SAMPLER";
> +
> +    if (!memorySampler && getenv(environMemorySampler)) {
> +        WKRetainPtr<WKDoubleRef> interval(AdoptWK, WKDoubleCreate(0.0));
> +        WKContextStartMemorySampler(ewk_context_WKContext_get(context), interval.get());
> +        memorySampler = true;
> +    }
> +#endif

Is this related to context? I think that it should be in ewk_context.

> Source/cmake/OptionsEfl.cmake:81
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEMORY_SAMPLER ON)

I am not sure whether we can add this.
But I think that we are using GLIB_SUPPORT for similar reason, 
So it can be reasonable although this option is only WebKit2/MAC and WebKit2/Efl now.

Anyway, to add this, you should also put this in WebKitFeatures.cmake and cmakeconfig.cmake.

[Mikhail Pozdnyakov]

Comment on attachment 160672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160672&action=review

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:56
> +const int maxBuffer = 128;

Do you want it to be exported? if not should be static or inside a nameless namespace.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:59
> +inline String getToken(FILE* file)

ditto. Even though 'inline' may have similar qualities as 'static' this behavior is compiler(and even compiler setting)-specific.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:71
>> +inline void appendMemoryStats(WebMemoryStatistics& webKitMemoryStats, String key, size_t value)
> 
> const String& key

ditto.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:77
> +void sampleSystemMemoryInfo(WebMemoryStatistics& statics)

ditto.

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:99
> +ApplicationMemoryStats sampleApplicationMalloc()

ditto.

[Gyuyoung Kim]

Comment on attachment 160672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160672&action=review

> Source/WebKit2/PlatformEfl.cmake:24
> +    Shared/WebMemorySampler.h

CMake doesn't include .h file.

[JungJik Lee]

Created attachment 160897 [details]
Patch

[JungJik Lee]

Comment on attachment 160672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160672&action=review

>> Source/WebKit2/PlatformEfl.cmake:24
>> +    Shared/WebMemorySampler.h
> 
> CMake doesn't include .h file.

done, thanks your comment.

>> Source/WebKit2/PlatformEfl.cmake:189
>> +    -DENABLE_MEMORY_SAMPLER=1
> 
> This is not needed if you want to add it in OptionsTizen.cmake.

done. thanks.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:56
>> +const int maxBuffer = 128;
> 
> Do you want it to be exported? if not should be static or inside a nameless namespace.

done.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:59
>> +inline String getToken(FILE* file)
> 
> ditto. Even though 'inline' may have similar qualities as 'static' this behavior is compiler(and even compiler setting)-specific.

done.

>>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:71
>>> +inline void appendMemoryStats(WebMemoryStatistics& webKitMemoryStats, String key, size_t value)
>> 
>> const String& key
> 
> ditto.

done.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:77
>> +void sampleSystemMemoryInfo(WebMemoryStatistics& statics)
> 
> ditto.

thanks your comments.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:106
>> +    if (fMemoryStatus) {
> 
> empty line is preffered.

done.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:118
>> +    return applicationStats;
> 
> need empty line.

done.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:134
>> +    return String();
> 
> need empty line.

done.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:748
>> +#endif
> 
> Is this related to context? I think that it should be in ewk_context.

it's better to stay in view. because memory sampler starts when view is created. I think it's nothing related with context. though it's using it.

>> Source/cmake/OptionsEfl.cmake:81
>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEMORY_SAMPLER ON)
> 
> I am not sure whether we can add this.
> But I think that we are using GLIB_SUPPORT for similar reason, 
> So it can be reasonable although this option is only WebKit2/MAC and WebKit2/Efl now.
> 
> Anyway, to add this, you should also put this in WebKitFeatures.cmake and cmakeconfig.cmake.

following your comment, I added defines to cmakeconfig.cmake and WebKitFeatures.cmake.

[Simon Hausmann]

Comment on attachment 160897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160897&action=review

> Source/WebKit2/PlatformEfl.cmake:26
> +    Shared/efl/WebMemorySamplerEfl.cpp

It seems that the implementation of the Sampler is not really EFL specific but rather Linux specific and perhaps instead belongs into Shared/linux/WebMemorySamplerLinux.cpp, for re-use by other ports. Then you can also get rid of the #if OS(LINUX) :)

> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:83
> +    bool foundKeyName = false;
> +    FILE* fSystemMemoryStatus = fopen("/proc/meminfo", "r");
> +    if (fSystemMemoryStatus) {
> +        while (!feof(fSystemMemoryStatus)) {

I'm inclined to say that it's more typical WebKit style to use early returns, i.e. instead of

FILE* f = fopen(...);
if (f) {
    ...
}

using

FILE* f = fopen(...);
if (!f)
    return;
...

and therefore achieving a lower level of indentation and presumably more readable code :)

[JungJik Lee]

Comment on attachment 160897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160897&action=review

>> Source/WebKit2/PlatformEfl.cmake:26
>> +    Shared/efl/WebMemorySamplerEfl.cpp
> 
> It seems that the implementation of the Sampler is not really EFL specific but rather Linux specific and perhaps instead belongs into Shared/linux/WebMemorySamplerLinux.cpp, for re-use by other ports. Then you can also get rid of the #if OS(LINUX) :)

EFL also has own ports. ex) mac, linux and windows. so even though I change the file name to WebMemorySamplerLinux.cpp, #if OS(LINUX) is still needed inside the cpp file for EFL. Or I should separate cmake file.

>> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:83
>> +        while (!feof(fSystemMemoryStatus)) {
> 
> I'm inclined to say that it's more typical WebKit style to use early returns, i.e. instead of
> 
> FILE* f = fopen(...);
> if (f) {
>     ...
> }
> 
> using
> 
> FILE* f = fopen(...);
> if (!f)
>     return;
> ...
> 
> and therefore achieving a lower level of indentation and presumably more readable code :)

Thanks your help. I'll fix it now.

[JungJik Lee]

Created attachment 160915 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 160915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160915&action=review

(In reply to comment #39)
 
> EFL also has own ports. ex) mac, linux and windows. so even though I change the file name to WebMemorySamplerLinux.cpp, #if OS(LINUX) is still needed inside the cpp file for EFL. Or I should separate cmake file.


EFL port already uses linux file as below,
http://trac.webkit.org/browser/trunk/Source/WebCore/PlatformEfl.cmake#L77

As you may know, EFL port mainly supports linux platform so far. Of course, EFL port will may support other platforms, for example, window, mac and so on. However, I think we can support it in CMake file using platform macro. As far as I know, WebKit culturally has shared common file with many ports. So, in my opinion, this patch needs to be considered to move linux directory.

> Source/JavaScriptCore/ChangeLog:3
> +        [EFL][WK2][CMAKE] Add WebMemorySampler option.

You should keep to be sync with bug title.

[JungJik Lee]

Created attachment 160928 [details]
Patch

[JungJik Lee]

Comment on attachment 160915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160915&action=review

>> Source/JavaScriptCore/ChangeLog:3
>> +        [EFL][WK2][CMAKE] Add WebMemorySampler option.
> 
> You should keep to be sync with bug title.

all done. Thanks your guide.

[Simon Hausmann]

Comment on attachment 160928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160928&action=review

> Source/WebKit2/PlatformEfl.cmake:33
> +    Shared/linux/WebMemorySamplerLinux.cpp

Excellent! :)

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:96
> +static void sampleSystemMemoryInfo(WebMemoryStatistics& statics)
> +{
> +    bool foundKeyName = false;
> +    FILE* fSystemMemoryStatus = fopen("/proc/meminfo", "r");
> +    if (!fSystemMemoryStatus)
> +        return;
> +
> +    while (!feof(fSystemMemoryStatus)) {
> +        String strToken = getToken(fSystemMemoryStatus);
> +        if (strToken.find(':') != notFound) {
> +            String keyName = strToken.left(strToken.length() - 1);
> +            statics.keys.append(keyName);
> +            foundKeyName = true;
> +        } else if (foundKeyName) {
> +            statics.values.append(strToken.toInt());
> +            foundKeyName = false;
> +        }
> +    }
> +    fclose(fSystemMemoryStatus);
> +}

I think instead of parsing /proc/meminfo you could use sysinfo(2) to get the same information with simpler code (no need to use insecure fscanf).

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:103
> +    char processPath[maxProcessPath];
> +    snprintf(processPath, maxProcessPath, "/proc/%d/statm", getpid());
> +    FILE* fMemoryStatus = fopen(processPath, "r");

I think you can simplify this to

    FILE* fMemoryStatus = fopen("/proc/self/statm", "r");

or alternatively get try to get the same information using getrusage(2) and getrlimit(2).

I think a programmatic approach is nicer than parsing files in /proc.

[JungJik Lee]

Comment on attachment 160928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160928&action=review

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:96
>> +}
> 
> I think instead of parsing /proc/meminfo you could use sysinfo(2) to get the same information with simpler code (no need to use insecure fscanf).

this is really good, thanks your help. I didn't know there is such an API for linux. I will replace the code with the API.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:103
>> +    FILE* fMemoryStatus = fopen(processPath, "r");
> 
> I think you can simplify this to
> 
>     FILE* fMemoryStatus = fopen("/proc/self/statm", "r");
> 
> or alternatively get try to get the same information using getrusage(2) and getrlimit(2).
> 
> I think a programmatic approach is nicer than parsing files in /proc.

I googled the usage of getrusage(2) and getrlimit(2) but I am not sure that I can replace info which is used now.
getrusage(2) gives some data but some fields of the data is unused on linux. getrlimit(2) is for getting the limitation of the resource the device provides.
I was also worried about using fscanf. so I'll change the code by not using fscanf for security.

[Ryuan Choi]

(In reply to comment #37)
> (From update of attachment 160672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160672&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:748
> >> +#endif
> > 
> > Is this related to context? I think that it should be in ewk_context.
> 
> it's better to stay in view. because memory sampler starts when view is created. I think it's nothing related with context. though it's using it.
> 

I still believe that it should not be in ewk_view because WebMemorySampler collects information per webprocess.

For example, we can create ewk_view from new context (which is not default context) and new context may have a way to start sampler.

Anyway, why don't you add this as ewk API to enable/disable at run-time?
I think that it looks more useful.

[JungJik Lee]

Comment on attachment 160672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160672&action=review

>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:748
>>>> +#endif
>>> 
>>> Is this related to context? I think that it should be in ewk_context.
>> 
>> it's better to stay in view. because memory sampler starts when view is created. I think it's nothing related with context. though it's using it.
> 
> I still believe that it should not be in ewk_view because WebMemorySampler collects information per webprocess.
> 
> For example, we can create ewk_view from new context (which is not default context) and new context may have a way to start sampler.
> 
> Anyway, why don't you add this as ewk API to enable/disable at run-time?
> I think that it looks more useful.

if you see this (https://bugs.webkit.org/attachment.cgi?id=153474&action=review), it was early code. I had same idea but Gyuyoung said there is no such APIs for debugging or profiling. So I removed it. I can move the code to context creation.

[JungJik Lee]

Created attachment 161218 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 161218 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161218&action=review

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:82
> +static inline void appendMemoryStats(WebMemoryStatistics& webKitMemoryStats, const String& key, size_t value)

Statistics?

Why webKitMemoryStats and not just statistics... the class name already makes it clear what it is

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:86
> +{
> +    webKitMemoryStats.keys.append(key);
> +    webKitMemoryStats.values.append(value);
> +}

This methods seems a bit useless.

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:88
> +static ApplicationMemoryStats sampleApplicationMalloc()

is malloc really the best word? Especially given that this is not tied to the malloc call?

sampleMemoryAllocatedForApplication ?

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:93
> +    FILE* fMemoryStatus = fopen(processPath, "r");

Why not statmFileDescriptor

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:97
> +    applicationStats.totalProgramSize = getToken(fMemoryStatus).toInt();

nextToken seems like a better name

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:104
> +    applicationStats.dirtyPageSize = getToken(fMemoryStatus).toInt();
> +    fclose(fMemoryStatus);

add a newline before fclose to make it easier to spot

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:124
> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const

WebKit is the project name and the name of the API's. Maybe sampleWebRuntime() makes more sense?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:90
> +        static bool memorySampler = false;

memory sampler doesn't sounds like a "booload" to me... sampleMemory?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:91
> +        static const char environMemorySampler[] = "MEMORY_SAMPLER";

environ? enviromentVariable? or ?

[JungJik Lee]

Comment on attachment 161218 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161218&action=review

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:82
>> +static inline void appendMemoryStats(WebMemoryStatistics& webKitMemoryStats, const String& key, size_t value)
> 
> Statistics?
> 
> Why webKitMemoryStats and not just statistics... the class name already makes it clear what it is

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:86
>> +}
> 
> This methods seems a bit useless.

it makes lines in sampleWebKit function be shorten. :)

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:88
>> +static ApplicationMemoryStats sampleApplicationMalloc()
> 
> is malloc really the best word? Especially given that this is not tied to the malloc call?
> 
> sampleMemoryAllocatedForApplication ?

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:93
>> +    FILE* fMemoryStatus = fopen(processPath, "r");
> 
> Why not statmFileDescriptor

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:97
>> +    applicationStats.totalProgramSize = getToken(fMemoryStatus).toInt();
> 
> nextToken seems like a better name

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:104
>> +    fclose(fMemoryStatus);
> 
> add a newline before fclose to make it easier to spot

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:124
>> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const
> 
> WebKit is the project name and the name of the API's. Maybe sampleWebRuntime() makes more sense?

I think so but mac port uses the same name and I could not find where they are using this function. so I could not change it by myself. :)

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:90
>> +        static bool memorySampler = false;
> 
> memory sampler doesn't sounds like a "booload" to me... sampleMemory?

MemorySampler is already using as a file and class name, so how about initializeMemorySampler?

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:91
>> +        static const char environMemorySampler[] = "MEMORY_SAMPLER";
> 
> environ? enviromentVariable? or ?

done! thanks your comments.

[JungJik Lee]

Created attachment 161254 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 161254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161254&action=review

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:67
> +    int index = 0;

unsigned?

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:70
> +        if (ch == EOF || (isASCIISpace(ch) && foundPrintable))

You don't need foundPrintable, you can just look at the index instead.

if (ch == EOF || isASCIISpace(ch) && index) // Break on non-initial ASCII space.
   break;

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:82
> +static inline void appendMemoryStatistics(WebMemoryStatistics& webKitMemoryStats, const String& key, size_t value)

I would just call the argument "stats" it seems more correct

static inline void appendKeyValuePair(WebMemoryStatistics& stats, const String& key, size_t value)


Anyway I think I find this more readable (from the *Mac version)

    webKitMemoryStats.keys.append(String("Timestamp"));
    webKitMemoryStats.values.append(now);
    webKitMemoryStats.keys.append(String("Total Bytes of Memory In Use"));
    webKitMemoryStats.values.append(totalBytesInUse);

It results in shorter, easy to read lines

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:126
> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const

I guess I would just call this sample(), but I see that it is used elsewhere, so fine

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:136
> +    FastMallocStatistics fastMallocStatistics = WTF::fastMallocStatistics();

What is fast malloc is disabled (common on mobile platforms where is often ends up using too much memory in comparison with the performance gain)

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:158
> +    appendMemoryStatistics(webKitMemoryStats, String("Shared"), applicationStats.sharedSize);

You should use ASCIILiteral("Shared"), it is faster. http://trac.webkit.org/wiki/EfficientStrings

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:164
> +#if USE(JSC)

Why can't these two USE ifdefs not be joined?

[Gyuyoung Kim]

Comment on attachment 161254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161254&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        [EFL][WK2][CMAKE] Add WebMemorySampler feature.

I think "[EFL][WK2]" prefix is correct.

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:61
> +    ASSERT(file);

I think this is unnecessary double checking because *file* is being checked if it is null just below.

> ChangeLog:3
> +        [EFL][WK2][CMAKE] Add WebMemorySampler feature.

ditto.

[Kenneth Rohde Christiansen]

Comment on attachment 161254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161254&action=review

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:61
>> +    ASSERT(file);
> 
> I think this is unnecessary double checking because *file* is being checked if it is null just below.

Maybe it is not, If it is not intented to be null adding an assert and a return is fine. The return could be to make sure this doesn't crash if there is someone using the API wrongly.

[EFL EWS Bot]

Comment on attachment 161254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161254&action=review

>>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:61
>>> +    ASSERT(file);
>> 
>> I think this is unnecessary double checking because *file* is being checked if it is null just below.
> 
> Maybe it is not, If it is not intented to be null adding an assert and a return is fine. The return could be to make sure this doesn't crash if there is someone using the API wrongly.

I'd like to understand exactly what did you mean. I meant ASSERT can be removed because file parameter is checked by *if (!file)* condition below. Do you think ASSERT is still needed ?

[Gyuyoung Kim]

(In reply to comment #55)
> (From update of attachment 161254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161254&action=review
> 
> >>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:61
> >>> +    ASSERT(file);
> >> 
> >> I think this is unnecessary double checking because *file* is being checked if it is null just below.
> > 
> > Maybe it is not, If it is not intented to be null adding an assert and a return is fine. The return could be to make sure this doesn't crash if there is someone using the API wrongly.
> 
> I'd like to understand exactly what did you mean. I meant ASSERT can be removed because file parameter is checked by *if (!file)* condition below. Do you think ASSERT is still needed ?

I understand Kenneth's opinion. ASSERT is able to help to find the wrong use in debug build as well as to expose problems. Thanks.

[JungJik Lee]

Comment on attachment 161254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161254&action=review

>> Source/JavaScriptCore/ChangeLog:3
>> +        [EFL][WK2][CMAKE] Add WebMemorySampler feature.
> 
> I think "[EFL][WK2]" prefix is correct.

done!

>>>>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:61
>>>>> +    ASSERT(file);
>>>> 
>>>> I think this is unnecessary double checking because *file* is being checked if it is null just below.
>>> 
>>> Maybe it is not, If it is not intented to be null adding an assert and a return is fine. The return could be to make sure this doesn't crash if there is someone using the API wrongly.
>> 
>> I'd like to understand exactly what did you mean. I meant ASSERT can be removed because file parameter is checked by *if (!file)* condition below. Do you think ASSERT is still needed ?
> 
> I understand Kenneth's opinion. ASSERT is able to help to find the wrong use in debug build as well as to expose problems. Thanks.

it was intended. I have same idea with kenneth. thanks all.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:67
>> +    int index = 0;
> 
> unsigned?

done!

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:70
>> +        if (ch == EOF || (isASCIISpace(ch) && foundPrintable))
> 
> You don't need foundPrintable, you can just look at the index instead.
> 
> if (ch == EOF || isASCIISpace(ch) && index) // Break on non-initial ASCII space.
>    break;

sharp eyes! thanks your comment.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:82
>> +static inline void appendMemoryStatistics(WebMemoryStatistics& webKitMemoryStats, const String& key, size_t value)
> 
> I would just call the argument "stats" it seems more correct
> 
> static inline void appendKeyValuePair(WebMemoryStatistics& stats, const String& key, size_t value)
> 
> 
> Anyway I think I find this more readable (from the *Mac version)
> 
>     webKitMemoryStats.keys.append(String("Timestamp"));
>     webKitMemoryStats.values.append(now);
>     webKitMemoryStats.keys.append(String("Total Bytes of Memory In Use"));
>     webKitMemoryStats.values.append(totalBytesInUse);
> 
> It results in shorter, easy to read lines

yes, I knew that and my proposal code was same with mac. however kang-il suggested using this inline function. And I thought it was not so bad... :) Thank you for comment.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:158
>> +    appendMemoryStatistics(webKitMemoryStats, String("Shared"), applicationStats.sharedSize);
> 
> You should use ASCIILiteral("Shared"), it is faster. http://trac.webkit.org/wiki/EfficientStrings

done. thanks.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:164
>> +#if USE(JSC)
> 
> Why can't these two USE ifdefs not be joined?

done.

>> ChangeLog:3
>> +        [EFL][WK2][CMAKE] Add WebMemorySampler feature.
> 
> ditto.

done. thank your comment.

[JungJik Lee]

Created attachment 161479 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 161479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161479&action=review

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:150
> +    FastMallocStatistics fastMallocStatistics = WTF::fastMallocStatistics();
> +    size_t fastMallocBytesInUse = fastMallocStatistics.committedVMBytes - fastMallocStatistics.freeListBytes;
> +    size_t fastMallocBytesCommitted = fastMallocStatistics.committedVMBytes;
> +    totalBytesInUse += fastMallocBytesInUse;
> +    totalBytesCommitted += fastMallocBytesCommitted;

i like the fast malloc to use an ifdef as well

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:91
> +        static const char environmentVariable[] = "MEMORY_SAMPLER";

Wouldn't SAMPLE_MEMORY not be better? What does other ports use?

[JungJik Lee]

Comment on attachment 161479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161479&action=review

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:150
>> +    totalBytesCommitted += fastMallocBytesCommitted;
> 
> i like the fast malloc to use an ifdef as well

done!

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:91
>> +        static const char environmentVariable[] = "MEMORY_SAMPLER";
> 
> Wouldn't SAMPLE_MEMORY not be better? What does other ports use?

done! And mac port only uses this feature but I think they do not activate this feature currently as far as I know.

[JungJik Lee]

Created attachment 161509 [details]
Patch

[JungJik Lee]

Created attachment 161510 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161510 [details]
Patch

Clearing flags on attachment: 161510

Committed r127195: <http://trac.webkit.org/changeset/127195>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.