719584 - Build SQLite with -O2 on Windows

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak]]

We are likely hitting a msvc 2005 bug here.
When compiling SQLite on windows+jemalloc+PGO we crash badly in je_free invoked by sqlite3_bind_text().
The compiler bug theory is confirmed by the fact buidling with -O2 doesn't crash.

So the patch just drops to -O2 on Windows when we use jemalloc.
I'm going to measure eventual perf changes on Try, hoping to get decent results.

Note that we are already building with -O2 on Mac for another crash issue (bug 676499).

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak]]

I don't see changes in Windows Talos numbers, actually I also verified what happens without jemalloc, and it doesn't matter, without it we just crash with another stack that ends in msvcr80.dll, so I'll remove the MOZ_MEMORY part. Looks like this is just a bug hitting with msvc 2005, I have notified drh of all of our findings, he said their windows developer will look into it.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 590144 [details] [diff] [review]
patch v1.1

ehr...

Comment 5

[xunxun]

MSVC has -O3?
I think it is -Ox ?

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 590144 [details] [diff] [review]
patch v1.1

(In reply to xunxun from comment #5)
> MSVC has -O3?
> I think it is -Ox ?

Ehr, good point, I was thinking gcc-like. Indeed looks like the default setting is /O1... so I'm actually changing the optimization here, not lowering it.
Clearing the review while I try to figure out with build team if O2 may have bad effects.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Thanks for pointing out my silliness with O3! Updated the comment and discussed briefly with glandium about it.

Probably there is no way to know if this may have side effects till we actually try it. Landing on Monday would give us about 10 days of baking in central, and 6 weeks of baking in Aurora. That should give us a decent evaluation of eventual crashes increase.

So to sum up, Windows+O1+PGO causes crashes when sqlite3_bind_test() tries to free the internal copy of the string (we use SQLITE_TRANSIENT exactly to request it does its internal copy). And this is much much likely a msvc2005 PGO bug.

Comment 8

[Shawn Wilsher :sdwilsh]

Which talos tests did you check?

Comment 9

[Marco Bonardo [:mak]]

all the ones we run in central. The fact is this is going to improve performances since O1 (current) minimize size, while O2 (the new one) maximize speed, so the result should be equal or better. That's what talos shows (there is just a time shift between ts_max and ts_shutdown_max, but that is normal for how the test works, sadly)

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 590171 [details] [diff] [review]
patch v1.2

Review of attachment 590171 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a60f021bab00

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a60f021bab00