1316282 - sWebMSample symbol should not be exported

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

+++ This bug was initially created as a clone of Bug #1316280 +++
bloaty reports that sWebMSample symbol is exported, my guess is that it is not necessary.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Anthony, what do you think? Thanks

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

It does not need to be exported.

Comment 3

[Andi [:andi]]

Attachment: Bug 1316282 - do not expose sWebMSample in XUL.

Review commit: https://reviewboard.mozilla.org/r/105160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105160/

Comment 4

[Andi [:andi]]

Comment on attachment 8827483 [details]
Bug 1316282 - do not expose sWebMSample in XUL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105160/diff/1-2/

Comment 5

[Andi [:andi]]

added a try buidl in order to be sure that everything is OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd06be1751b64e6c2c0ace5893621083a0a9681

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8827483 [details]
Bug 1316282 - do not expose sWebMSample in XUL.

I'll let jya review this.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8827483 [details]
Bug 1316282 - do not expose sWebMSample in XUL.

https://reviewboard.mozilla.org/r/105160/#review107960

::: dom/media/WebMSample.h:1
(Diff revision 2)
> -static const uint8_t sWebMSample[] = {
> +uint8_t webMSample[] = {

this should still be const

Comment 8

[Andi [:andi]]

I specially omitted the ussage of keyword const, since in this context using const would have had the same effect as having:

>> static const uint8_t webMSample[] = {

that would have led to the exposure of the object outside of it's scope. 

     VM SIZE                                                             FILE SIZE
 --------------                                                       --------------
  61.8%   222Mi [None]                                                  222Mi  61.8%
  37.6%   135Mi [Other]                                                 134Mi  37.5%
   0.1%   412Ki (anonymous namespace)::gBucketLowerBounds               412Ki   0.1%
   0.1%   266Ki mozilla::VP9Benchmark::IsVP9DecodeFast()::webMSample    266Ki   0.1%
   0.1%   255Ki _ref30131                                               255Ki   0.1%
   0.1%   196Ki kSTSHostTable                                           196Ki   0.1%
   0.0%   135Ki _vpx_fdct32x32_avx2                                     135Ki   0.0%
   0.0%   119Ki _kBrotliDictionary                                      119Ki   0.0%
   0.0%   116Ki _encode_one_block                                       116Ki   0.0%
   0.0%   104Ki _vpx_fdct32x32_sse2                                     104Ki   0.0%
   0.0%   104Ki mozilla::a11y::PDocAccessibleChild::OnMessageReceived   104Ki   0.0%
   0.0%  89.0Ki Interpret                                              89.0Ki   0.0%
   0.0%  85.8Ki _ref26410                                              85.8Ki   0.0%
   0.0%  83.6Ki mozilla::ipc::StringFromIPCMessageType                 83.6Ki   0.0%
   0.0%  79.5Ki ETLDEntry::strings                                     79.5Ki   0.0%
   0.0%  74.8Ki _vpx_idct32x32_1024_add_sse2                           74.8Ki   0.0%
   0.0%  72.3Ki MOZ_ReportAssertionFailure                             72.3Ki   0.0%
   0.0%  71.7Ki _vpx_fdct32x32_rd_avx2                                 71.7Ki   0.0%
   0.0%  69.5Ki nsHtml5AttributeName::initializeStatics                69.5Ki   0.0%
   0.0%  69.5Ki mozilla::dom::PContentParent::OnMessageReceived        69.5Ki   0.0%
   0.0%  64.0Ki gfxUtils::sPremultiplyTable                            64.0Ki   0.0%

This behaviour is present since we store webMSample in a protected space of the memory that's exposed by the compiler.

Comment 9

[Andi [:andi]]

Comment on attachment 8827483 [details]
Bug 1316282 - do not expose sWebMSample in XUL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105160/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c429f0a0040e
do not expose sWebMSample in XUL. r=jya

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for making Windows PGO builds time out. https://hg.mozilla.org/integration/autoland/rev/5c5b0537712323cf1d9610a6fe1b127c65902182

Comment 12

[(Away)]

This change looks really suspicious.

I'm not familiar with bloaty but I would expect that its purpose is to point out large data structures so that you can _remove_ them, or otherwise refactor your code to avoid them. Simply _moving_ the data into a local variable doesn't change anything. sWebMSample still takes up the same amount of space in libxul. Except now there's this unpleasant mid-file #include.

Additionally I don't know what is bloaty's definition of "exported", but it's certainly not the same as mine. I wouldn't consider sWebMSample exported in any way.

IMO this should be a wontfix.

Comment 13

[(Away)]

(Unless, of course, you want to remove sWebMSample altogether, which would make me very happy from a binary-size perspective -- but it's probably out of scope for this bug. :) )

Comment 14

[Andi [:andi]]

(In reply to David Major [:dmajor] from comment #13)
> (Unless, of course, you want to remove sWebMSample altogether, which would
> make me very happy from a binary-size perspective -- but it's probably out
> of scope for this bug. :) )

I agree with you that this patch doesn't affect the size of XUL, but still we decrease the memory footprint.

Comment 15

[(Away)]

(In reply to Andi-Bogdan Postelnicu from comment #14)
> I agree with you that this patch doesn't affect the size of XUL, but still
> we decrease the memory footprint.

For my curiosity, can you please explain how this would decrease the memory footprint? That data still has to be somewhere.

Comment 16

[Andi [:andi]]

Yes indeed, but it will be allocated during the lifespan of the parent function. Yes there will be a style when that function gets called, caused by the allocation. But at least we don't keep it for the entire lifespan of xul.

Comment 17

[(Away)]

But the data _does_ live for the lifespan of xul.dll, because it's embedded _inside_ of xul.dll. From the compiler's perspective it doesn't matter what lifetime is used, because the generated code will still need to grab that data from somewhere in the binary.

Here's what it looks like before the change:

00001`8111f315 488d05c4c3ad01  lea     rax,[xul!sWebMSample (00000001`82bfb6e0)]
00000001`8111f31c 48c74618d52b0400 mov     qword ptr [rsi+18h],42BD5h
00000001`8111f324 48894610        mov     qword ptr [rsi+10h],rax
00000001`8111f328 488d55a8        lea     rdx,[rbp-58h]
00000001`8111f32c 48897e20        mov     qword ptr [rsi+20h],rdi
00000001`8111f330 e8a3dafeff      call    xul!mozilla::MediaExtendedMIMEType::MediaExtendedMIMEType 

Where 'sWebMSample' is a blob in the .rdata section of xul.dll:
0       0       273365  2       sWebMSample     

I applied the patch (had to remove 3/4 of the array to make the compiler not time out) and now the websample still lives in libxul, but in the .text section (implicitly, as part of the code) -- but the size is amplified by 8x(!) because the compiler uses an 8-byte instruction to write each byte of the sample:

00000001`81120254 c605ee51160201  mov     byte ptr [xul!mozilla::VP9Benchmark::sHasRunTest (00000001`83285449)],1
00000001`8112025b c684245001000072 mov     byte ptr [rsp+150h],72h
00000001`81120263 c684245101000054 mov     byte ptr [rsp+151h],54h
00000001`8112026b c68424520100008f mov     byte ptr [rsp+152h],8Fh
00000001`81120273 c684245301000065 mov     byte ptr [rsp+153h],65h
[...much more...]

So I really don't recommend doing this. :)

Comment 18

[Andi [:andi]]

I really appreciate in clarifying this out for me, this also explains why the win builds where timing out.