Closed Bug 987146 Opened 9 years ago Closed 9 years ago

Represent SQL queries more efficiently

Categories

(Firefox Health Report Graveyard :: Data Request, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 1 obsolete file)

Concatenation of JS string literals currently involves quadratic memory consumption at parse time. Multi-line string literals avoid this problem. Bug 986683 addresses the worst example, and the next worse example is all the SQL queries in storage.jsm.

Bug 986672 will fix the quadratic build-up, but even when that's done, concatenation is still less efficient than multi-line literals because the string fragments need to be created and atomized (which involves sticking them in a hash table) as well as the final concatenated string.
Whiteboard: [MemShrink]
Blocks: 986323
I did this semi-automatically, via some search-and-replace commands in my text
editor.

Are these statements tested? I tried doing a try push with deliberate syntax
errors inserted into a couple of the queries (see
https://tbpl.mozilla.org/?tree=Try&rev=75d36774479e) but I didn't get any
failures.
Attachment #8395847 - Flags: review?(gps)
Wait, what? I was told that SpiderMonkey handled string concatenation by essentially constructing a "thread" of references to existing strings. Concatenating strings via the + operator was effectively appending a string ref to a linked list (or similar O(1) insertion performance).

Am I wrong about this? Does the behavior vary between run-time and parse-time? Is any of this documented anywhere?
Flags: needinfo?(n.nethercote)
> Does the behavior vary between run-time and parse-time? 

Yes. The quadratic behaviour only happens at parse-time. Run-time concatenations use "ropes", which avoid this problem.

> Is any of this documented anywhere?

No.

BTW, I was wrong about the lack of test failures; I've got some browser-chrome tests failures to look into.
Flags: needinfo?(n.nethercote)
The previous patch had a small mistake. I'll give you a gold star if you can
find it.
Attachment #8396102 - Flags: review?(gps)
Attachment #8395847 - Attachment is obsolete: true
Attachment #8395847 - Flags: review?(gps)
The browser-chrome tests (which failed on my previous try run) are now green:
https://tbpl.mozilla.org/?tree=Try&rev=656546397e36
Comment on attachment 8396102 [details] [diff] [review]
Represent SQL queries more efficiently.

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

rs=me

I didn't look at each change in detail. But I have confidence in the test coverage of this file.

::: services/metrics/storage.jsm
@@ +156,5 @@
>   */
>  const SQL = {
>    // Create the providers table.
> +  createProvidersTable: "\
> +CREATE TABLE providers (\

The indentation here is different. What's the deal with whitespace on line continuations?

(Did I just earn a gold star?)
Attachment #8396102 - Flags: review?(gps) → review+
> The indentation here is different. What's the deal with whitespace on line
> continuations?

Imagine that you remove the trailing '\' chars and the newlines, and that's the string you get. So the patch does result in the strings having more whitespace than before. But that doesn't affect their meaning, and I figured keeping the indentation was more important.
https://hg.mozilla.org/mozilla-central/rev/7641cbf0ba8d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.