Nicholas Nethercote [inactive] Assignee
	Description • 14 years ago

Cachegrind tells me that js_NewFinalizableGCThing() is the SM function with
the highest calling overhead on both SS and V8, ie. the one most likely to benefit from being inlined.  So this patch partially inlines it.  It's what you might call "winning ugly".  Among the hoops jumped through:  METER/METER_IF had to become GCMETER/GCMETER_IF to avoid clashing with other definitions.

I don't know if I'd call it a good patch, but the results aren't bad:

---------------------------------------------------------------
| millions of instructions executed                           |
| total                        | on-trace (may overestimate)  |
---------------------------------------------------------------
|    99.181    98.718 (1.005x) |    25.810    25.768 (1.002x) | 3d-cube
|    47.262    47.251 (------) |    25.562    25.561 (------) | 3d-morph
|   113.190   112.825 (1.003x) |    44.603    44.573 (1.001x) | 3d-raytrace
|    68.899    67.974 (1.014x) |    17.070    16.986 (1.005x) | access-binary-
|   111.388   111.372 (------) |    92.571    92.570 (------) | access-fannkuc
|    37.555    37.549 (------) |    16.703    16.702 (------) | access-nbody
|    51.836    51.827 (------) |    31.633    31.632 (------) | access-nsieve
|    16.359    16.350 (1.001x) |     3.249     3.248 (------) | bitops-3bit-bi
|    45.921    45.912 (------) |    32.702    32.701 (------) | bitops-bits-in
|    24.783    24.774 (------) |    12.019    12.019 (------) | bitops-bitwise
|    56.401    56.392 (------) |    42.156    42.155 (------) | bitops-nsieve-
|    34.435    34.426 (------) |    21.298    21.297 (------) | controlflow-re
|    47.279    47.263 (------) |     6.174     6.173 (------) | crypto-md5
|    30.072    30.058 (------) |     7.021     7.020 (------) | crypto-sha1
|    96.784    95.546 (1.013x) |    17.536    17.421 (1.007x) | date-format-to
|    83.651    81.312 (1.029x) |     9.770     9.556 (1.022x) | date-format-xp
|    54.286    54.275 (------) |    31.293    31.292 (------) | math-cordic
|    29.558    29.792 (0.992x) |     6.232     6.462 (0.964x) | math-partial-s
|    31.048    31.034 (------) |    13.352    13.351 (------) | math-spectral-
|    58.534    58.515 (------) |    34.592    34.590 (------) | regexp-dna
|    39.761    38.181 (1.041x) |     9.409     9.277 (1.014x) | string-base64
|   116.829   116.249 (1.005x) |    26.066    25.997 (1.003x) | string-fasta
|   138.341   137.154 (1.009x) |    17.895    17.788 (1.006x) | string-tagclou
|   180.413   178.531 (1.011x) |    22.160    21.959 (1.009x) | string-unpack-
|    57.994    56.329 (1.030x) |    11.858    11.698 (1.014x) | string-validat
-------
|  1671.774  1659.622 (1.007x) |   578.747   577.810 (1.002x) | all


I'll attach Sunspider timings shortly.

	Luke Wagner [:luke]
	Comment 2 • 14 years ago

I think Gregor's big GC patch includes an inlined NewFinalizableGCThing.

	Igor Bukanov
	Comment 3 • 14 years ago

(In reply to comment #2)
> I think Gregor's big GC patch includes an inlined NewFinalizableGCThing.

This suggests in fact to land this bug first to clearly separate the performace benefits of the inlining from the compartment changes. In fact the SS win here matches the numbers from the bug 558861 comment 42.

	Igor Bukanov
	Comment 4 • 14 years ago

Comment on attachment 473873 [details] [diff] [review]
patch (against TM 53113:a59676d2e7b5)

This does not include jsgcinlines.h

> void *
>-js_NewFinalizableGCThing(JSContext *cx, unsigned thingKind)
>+js_NewFinalizableGCThingSlow(JSContext *cx, unsigned thingKind, JSGCThing **freeListp)
> {
>-    JS_ASSERT(thingKind < FINALIZE_LIMIT);
>-#ifdef JS_THREADSAFE
>-    JS_ASSERT(cx->thread);
>-#endif


This should assert that the free list is empty. Also keep the assertions for better coverage.

	Nicholas Nethercote [inactive] Assignee
	Comment 5 • 14 years ago

Patch addressing Igor's comments.  Landing the inlining separately is a reasonable idea, but it will cause conflicts with Gregor's patch, so I'm happy to abandon this bug and let it all happen in Gregor's bug.

	Gregor Wagner [:gwagner]
	Comment 6 • 14 years ago

Yeah merging is not fun. Nick please wait a few days and if my patch is not in by Tuesday or Wednesday feel free to land. Thanks!

	Nicholas Nethercote [inactive] Assignee
	Comment 7 • 14 years ago

Gregor's big GC patch landed, subsuming this bug.