| Summary: | [EFL][WK2] Add WebMemorySampler feature. | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | JungJik Lee <jungjik.lee> | ||||||||||||||||||||||||||||||||||||
| Component: | WebKit EFL | Assignee: | JungJik Lee <jungjik.lee> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, d-r, gyuyoung.kim, gyuyoung.kim, hausmann, lucas.de.marchi, rakuco, ryuan.choi, sw0524.lee, tmpsantos, webkit.review.bot | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=164517 | ||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
|
Description
JungJik Lee
2012-07-13 04:11:20 PDT
Created attachment 153226 [details]
proposal patch
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.
Created attachment 153233 [details]
fix style error.
Created attachment 153474 [details]
Patch
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? 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. (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. 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. Perhaps it's time to revisit this now that bug 91274 has been fixed? (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. (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. (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. Created attachment 158812 [details]
Patch
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. Created attachment 159015 [details]
Patch
Created attachment 160361 [details]
Patch
(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. 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. 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. Created attachment 160413 [details]
Patch
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()' 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. Created attachment 160467 [details]
Patch
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. 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. 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. :-) Comment on attachment 160467 [details]
Patch
I cq- because of my comments regarding Platform.h. This looks like a pure EFL feature.
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? Comment on attachment 160467 [details]
Patch
r- because of many comments.
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; }; Created attachment 160672 [details]
Patch
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? 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. 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. 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. Created attachment 160897 [details]
Patch
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. 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 :) 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. Created attachment 160915 [details]
Patch
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. Created attachment 160928 [details]
Patch
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. 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. 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. (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. 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. Created attachment 161218 [details]
Patch
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 ? 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. Created attachment 161254 [details]
Patch
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? 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. 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. 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 ? (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. 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. Created attachment 161479 [details]
Patch
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? 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. Created attachment 161509 [details]
Patch
Created attachment 161510 [details]
Patch
Comment on attachment 161510 [details] Patch Clearing flags on attachment: 161510 Committed r127195: <http://trac.webkit.org/changeset/127195> All reviewed patches have been landed. Closing bug. |