After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 643817 - gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead
gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2011-03-03 21:05 UTC by Colin Walters
Modified: 2011-03-10 20:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead (3.97 KB, patch)
2011-03-03 21:05 UTC, Colin Walters
none Details | Review
gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead (5.24 KB, patch)
2011-03-03 22:35 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2011-03-03 21:05:19 UTC
In commit 45b550d6790dba70, we introduced a function which
attempts to force a JavaScript GC if native malloc() had allocated
a lot.  The major flaw with this is that on medium size processes
(e.g. gnome-shell), mallinfo *is* slow.  I'd only benchmarked
it on smaller ones.

Rather than looking at malloc, a simpler and definitely faster
way to reflect on our process' memory impact is to look at RSS.

Also, don't call gjs_maybe_gc() internally; leave it up to
embedders to invoke manually.
Comment 1 Colin Walters 2011-03-03 21:05:22 UTC
Created attachment 182405 [details] [review]
gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead
Comment 2 Colin Walters 2011-03-03 22:35:41 UTC
Created attachment 182413 [details] [review]
gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead

Actually remove internal calls to gjs_maybe_gc()
Comment 3 Owen Taylor 2011-03-03 22:53:51 UTC
Review of attachment 182413 [details] [review]:

Code looks OK, but don't understand the idea, really.

::: gjs/jsapi-util.c
@@ +1585,3 @@
+         */
+        if (rss_size > linux_rss_trigger) {
+            linux_rss_trigger = (size_t) (rss_size * 1.25f);

Overthinking, hprobably OK because of the 3:1 process/kernel split, but it makes me a bit nervous on 32-bit... seem to remember people fooling around with 4GB process spaces somehow. Also, don't use floats (1.25f) unless you mean it.

 The (size_t) cast here is also odd since the variable is gulong. 

 (gulong)MIN(G_MAXULONG, rss_size * 1.25));

is what I'd write. Of course, if we do get that big, then we stop calling JS_GC, but probably Spidermonkey will do it itself if it gets an allocation failure.

But wai thuh, I don't understand the bumping up limit - this will cause the process to get bigger and bigger over time, even if we successfully collect garbage.
Comment 4 Colin Walters 2011-03-10 20:22:37 UTC
Attachment 182413 [details] pushed as 7aae32d - gjs_maybe_gc: mallinfo() is slow, look at /proc/self/statm instead