If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

JSRuntime local variables increases code size on x86

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED WONTFIX
10 years ago
5 years ago

People

(Reporter: Igor Bukanov, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
[This is a spin-off of bug 378793 comment 55]

Due to register pressure on x86 using a local JSRuntime *rt to store cx->runtime is counterproductive since a compiler will use a stack slot to store cx->runtime while for cx the compiler can assign a register. Thus to access cx->runtime the compiler uses similar offset(cx_register) or offset(stack_register) instructions and initial storage of cx->runtime in the stack slot for rt is just a waste of space.

I also suspect that even on more register friendly architectures then x86 using rt locals can, in fact, lead to code bloat since the compiler will have to preserve the register when calling a function from a different compilation unit. In this case the compiler can not assume that cx->runtime is not modified so it is not possible to use cx->runtime to reload rt register if the latter was overwritten.
(Reporter)

Comment 1

10 years ago
Created attachment 268473 [details] [diff] [review]
implementation v1

The patch replaces rt via cx->runtime in the interpreter and saves 30-40 bytes from stripped jsinter.o with GCC 4.1.2 at -O, -O2, -Os when targeting a generic 586 CPU. With explicit -march=pentium-m the difference growth to 50-60 bytes.

It would be interesting to know the results on x64.
Attachment #268473 - Flags: review?(brendan)

Comment 2

10 years ago
With VC8 /O2 for x86 the patch increases the size of jsinterp.obj by 6 bytes. There are two more debug-only uses of rt in js_interpret in JS_RUNTIME_METER macros.
Comment on attachment 268473 [details] [diff] [review]
implementation v1

Waiting for new patch per Seno.Aiko's comment.

/be
Attachment #268473 - Flags: review?(brendan)
(Reporter)

Comment 4

10 years ago
I am not working on the bug right now.
Assignee: igor → general

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.