jsshell: wrapper for JS_SetGCParameter

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
For measuring performance of the GC it would be nice to be able to call from a script JS_SetGCParameter to change JSGC_MAX_BYTES and JSGC_MAX_MALLOC_BYTES as suitable.
(Assignee)

Comment 1

11 years ago
Created attachment 286297 [details] [diff] [review]
v1

The patch adds gcparam to jsshell, a wrapper for JS_SetGCParameter.
Attachment #286297 - Flags: review?
(Assignee)

Comment 2

11 years ago
Created attachment 286300 [details] [diff] [review]
v2

The next time I should remember that for fast natives the return value must be set in any case.
Attachment #286297 - Attachment is obsolete: true
Attachment #286300 - Flags: review?(brendan)
Attachment #286297 - Flags: review?
Comment on attachment 286300 [details] [diff] [review]
v2

>+        fprintf(gErrFile,
>+                "gcparam: argument 1 must be either "
>+                "maxBytes or maxMallocBytes\n");

This should be a JS_ReportError call -- also it should imitate other such calls by not including "gcparam: " at the front. Final nit: "first argument" is better than argument 1.

>+        fprintf(gErrFile,
>+                "gcparam: argument 2 must be convertable to uint32 with "
>+                "non-zero value\n");

Ditto.

r=me with these changes.

/be
Attachment #286300 - Flags: review?(brendan) → review+
(Assignee)

Comment 4

11 years ago
Created attachment 286437 [details] [diff] [review]
v3

Here is the patch to commit with nits addressed. I also replaced fprintf by JS_ReportError in the implementation of countHeap and dumpHeap shell functions.
Attachment #286300 - Attachment is obsolete: true
Attachment #286437 - Flags: review+
(Assignee)

Comment 5

11 years ago
I checked in the patch from comment 4 to CVS trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.169; previous revision: 3.168
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 6

11 years ago
peterv backed this out on 20071028. Shouldn't we reopen it?
Bah, that's not what I meant to do. I'll reland this tomorrow.
Undid my backout and relanded this.

Comment 9

11 years ago
do you think it would be worth it to expose this to the command line js and jsDriver so we could run the test suite with specific values?
Flags: in-testsuite-
Flags: in-litmus-
(Assignee)

Comment 10

11 years ago
(In reply to comment #9)
> do you think it would be worth it to expose this to the command line js and
> jsDriver so we could run the test suite with specific values?

The function is already exposed through -e option as in:

js -e 'gcparam("maxBytes", 100<<10)' <other options and arguments>

Theoretically it is not ideal as it executes JS code which may be affected by other options. But I suggest to worry about it when we get a test case where -e would not be enough.
You need to log in before you can comment on or make changes to this bug.