19459 - Bloat in generated code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Daniel van der Zee]

(Probably not the right component?)

The classes that define the DOM classes for JavaScript in /dom/src/ seem highly
ineffecient to me. The methods defined there contain case statements that is a
repetition of the following code (from GetProperty).

  case HTMLTEXTAREAELEMENT_DISABLED:
  {
	PRBool ok = PR_FALSE;
	secMan->CheckScriptAccess(scriptCX, obj,
NS_DOM_PROP_HTMLTEXTAREAELEMENT_DISABLED, PR_FALSE, &ok);
	if (!ok) {
	  return nsJSUtils::nsReportError(cx, NS_ERROR_DOM_SECURITY_ERR);
	}
	PRBool prop;
	nsresult result = NS_OK;
	result = a->GetDisabled(&prop);
	if (NS_SUCCEEDED(result)) {
	  *vp = BOOLEAN_TO_JSVAL(prop);
	}
	else {
	  return nsJSUtils::nsReportError(cx, result);
	}
	break;
  }

I think that everything but the "result = a->GetDisabled(&prop)" could be moved
to outside of the case statement

Did a quick search and there appear to be about 1200 of these code blocks across
100 files. Assuming the code savings is 50 bytes per case label, the resulting
binary could by reduced in size by over 50K.

These files are generated, I couldn't find what generates them (is that code
available?). Point me to the generating code and I'll try to see how much space
is actually saved by being a little more intelligent.

Comment 1

[Daniel van der Zee]

Attachment: nsJSHTMLButtonElement.cpp (sample)

Comment 2

[Daniel van der Zee]

> little more intelligent.
Err, sorry. I hope that sounds OK, I mean something like "trying to be more
space efficent" or something. I very much appreciate all the work that you guys
do in the open.

Looked at this again and it appears to be more complex than I first thought I
may have (a start of) an implementation that reduces code size.

Attached a file that contains some generic code for a function that can be
re-used for *getting* DOM properties. Since I don't fully understand the build
system, I put all code into nsJSHTMLButtonElement.cpp as an example. The code
compiles but I did not test if it actually works.

The implementation uses a lookup table that lists for each property: the type,
security id and pointers to getter and setter methods. The generic property
getter just accepts a pointer to this table and does its magic based on the
index of the property.

I could not find an example for a property *setter* that accepts a DOM type so
I'm not sure what information is needed for that. It might be fairly complex and
require an IID added in the lookup table.

Comment 3

[Daniel van der Zee]

Sorry for using bugzilla as a personal scratchpad but I found the answer to most
of the questions in this bug. I changed the code generation to generate calls
into a single generic function for property getters. This saved 70K in the
optimized build of jsdom.dll (out of 370K). If I can find the time during next
week, I'll try to do the same thing for setters (I expect another 70K savings).

Assigned this to myself. Please stop me if I overlooked something important
(e.g. member pointers don't work XP).

Comment 4

[Daniel van der Zee]

Vidur,

People on #mozilla suggested I contact you about this one. If you have the time,
could you maybe take a look at this bug and see if I'm ovf=erlooking something
in this?

Comment 5

[Daniel van der Zee]

I'll abondon this for the time being, out of time, leaving on a 4 week vacation
and jband seems to be doing a dom xpconnect overhaul for m13. Please close at
next cleanup or do something with the attached files.

Basic idea:
- Generate lookup tables for all properties (using member pointers)
- Use generic property/get set routines from the lookup tables

Saves about 90K on jsdom.dll for a release build on win32. It seems to work
(browser still starts), don't remember if all testcases work.

Note on the attachment:
- changed tools/JSStubGen.*
- added src/base/nsDOMJSUtils.*

did not include changed Makefiles.

Comment 6

[Daniel van der Zee]

Attachment: tar file

Comment 7

[vidur (gone)]

Sorry this fell off my list. You're right - there's potential for bloat
reduction in the generated code. I'll check out your suggestions over the next
couple of weeks to see what we might be able to do for M13. Thanks!

Comment 8

[Blake Ross]

[e-mail], are you still around?  Vidur, do you want to take this if 
not, or should we lcose this up?

Comment 9

[Daniel van der Zee]

Yeah. I'm still around but I currently don't have the time to contribute. If I 
remember correctly, the basic story is that this bug describes some potential 
for saving space in the js <-> dom bridge. 

The best space saving is ofcourse getting rid of the entire bridge and just use 
xpconnect. I believe bug 28833 describes what needs to be done. Maybe this bug 
should be marked a duplicate of bug 28833?

Comment 10

[Daniel van der Zee]

Completely irrelevant right now since the JS<->DOM thing (forgot the name, 
xpconnect maybe?) has landed.

Comment 11

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/56c6762c0b4d
[fenix] Close https://github.com/mozilla-mobile/fenix/issues/19459: Use Store.waitUntilIdle in intermittent failing test (https://github.com/mozilla-mobile/fenix/pull/19683)