Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve scalar quoting performance #69

Closed

Conversation

juergenhoetzel
Copy link

Results on my system:

Current:

(benchmark 100000 '(emacsql-quote-scalar "I'm learning Python."))
"Elapsed time: 2.657825s (1.080004s in 23 GCs)"

PR:

(benchmark 100000 '(emacsql-quote-scalar "I'm learning Python."))
"Elapsed time: 1.067836s (0.787702s in 17 GCs)"

@skeeto
Copy link
Contributor

skeeto commented Nov 22, 2020

The original implementation was based on replace-regexp-in-string: 3b70e8f. I switched to the temp buffer version because whatever throwaway benchmark I used at the time showed it was faster, particularly because replace-regexp-in-string generates tons of garbage.

Revisiting it now with Emacs 27.1, there's a crossover point between 3 and 4 apostrophes. The temp buffer has a high startup cost, but is overall more efficient (the gap buffer avoids generating garbage).

;;; -*- lexical-binding: t; -*-
(dotimes (i 25)
  (let ((s (format "abc%sdef" (make-string i ?'))))
    (benchmark 100000 (list 'emacsql-quote-scalar s))))

time

The regexp-replace version runs the garbage collector much more often:

gc

No matter the number of apostrophes, it spends most of its time in the garbage collector:

gct

Since most string scalars are expected to have fewer than 4 apostrophes, it probably makes sense just to use the replace-regexp version, but I'm still wary about the worst case and generating so much garbage.

@juergenhoetzel juergenhoetzel force-pushed the scalar-quote-performance branch from 4bea632 to 97b866d Compare November 23, 2020 12:08
@juergenhoetzel
Copy link
Author

Thank you for the detailed insights and explanation of the historical reasons that led to the current implementation.

Interestingly, the replace-regex implementation on my system is always faster. Anyway, I have looked at the implementation of replace-regex and now I have implemented a specialized version, which is much faster:

regexp-replace                                   temp-buffer                                      own implementation

Elapsed time: 0.521850s (0.524323s in 8 GCs)     Elapsed time: 2.559998s (1.298774s in 28 GCs)    Elapsed time: 0.443598s (0.333841s in 5 GCs)
Elapsed time: 0.767311s (0.703203s in 13 GCs)    Elapsed time: 2.969729s (1.616639s in 33 GCs)    Elapsed time: 0.686606s (0.538863s in 10 GCs)
Elapsed time: 1.061757s (0.913401s in 17 GCs)    Elapsed time: 3.216180s (1.788128s in 38 GCs)    Elapsed time: 0.812525s (0.611451s in 11 GCs)
Elapsed time: 1.413832s (1.185098s in 22 GCs)    Elapsed time: 3.816427s (2.116909s in 43 GCs)    Elapsed time: 0.842065s (0.695212s in 13 GCs)
Elapsed time: 1.704233s (1.399007s in 26 GCs)    Elapsed time: 4.111683s (2.391927s in 49 GCs)    Elapsed time: 0.948747s (0.748723s in 14 GCs)
Elapsed time: 2.054108s (1.668513s in 31 GCs)    Elapsed time: 4.198231s (2.453352s in 54 GCs)    Elapsed time: 1.053291s (0.800285s in 15 GCs)
Elapsed time: 2.357662s (1.882876s in 35 GCs)    Elapsed time: 4.572700s (2.619197s in 58 GCs)    Elapsed time: 1.267134s (0.854599s in 16 GCs)
Elapsed time: 2.651335s (2.098092s in 39 GCs)    Elapsed time: 5.015285s (2.917136s in 64 GCs)    Elapsed time: 1.318649s (0.960398s in 18 GCs)
Elapsed time: 3.002761s (2.367188s in 44 GCs)    Elapsed time: 5.384789s (3.157256s in 69 GCs)    Elapsed time: 1.426605s (1.015898s in 19 GCs)
Elapsed time: 3.298794s (2.583839s in 48 GCs)    Elapsed time: 6.189858s (3.695950s in 75 GCs)    Elapsed time: 1.638877s (1.068143s in 20 GCs)
Elapsed time: 3.653449s (2.860977s in 53 GCs)    Elapsed time: 6.193920s (3.765986s in 79 GCs)    Elapsed time: 1.690573s (1.175211s in 22 GCs)
Elapsed time: 3.985965s (3.077324s in 57 GCs)    Elapsed time: 6.444607s (3.869025s in 84 GCs)    Elapsed time: 1.796182s (1.227437s in 23 GCs)
Elapsed time: 4.228551s (3.292603s in 61 GCs)    Elapsed time: 6.861403s (4.213131s in 90 GCs)    Elapsed time: 2.010033s (1.280468s in 24 GCs)
Elapsed time: 4.579165s (3.552230s in 66 GCs)    Elapsed time: 7.236405s (4.441292s in 95 GCs)    Elapsed time: 2.068538s (1.388774s in 26 GCs)
Elapsed time: 4.988655s (3.771820s in 70 GCs)    Elapsed time: 7.431056s (4.595113s in 100 GCs)   Elapsed time: 2.215706s (1.444392s in 27 GCs)
Elapsed time: 5.239030s (4.038972s in 75 GCs)    Elapsed time: 7.802493s (4.873736s in 105 GCs)   Elapsed time: 2.393896s (1.497034s in 28 GCs)
Elapsed time: 5.526718s (4.253166s in 79 GCs)    Elapsed time: 8.137921s (5.080095s in 110 GCs)   Elapsed time: 2.440634s (1.600902s in 30 GCs)
Elapsed time: 5.929200s (4.468238s in 83 GCs)    Elapsed time: 8.626883s (5.356165s in 115 GCs)   Elapsed time: 2.547778s (1.655972s in 31 GCs)
Elapsed time: 6.272569s (4.738194s in 88 GCs)    Elapsed time: 8.847913s (5.546497s in 121 GCs)   Elapsed time: 2.758850s (1.706297s in 32 GCs)
Elapsed time: 6.551936s (5.034335s in 93 GCs)    Elapsed time: 9.078585s (5.633488s in 125 GCs)   Elapsed time: 2.815046s (1.816697s in 34 GCs)
Elapsed time: 6.862757s (5.165854s in 96 GCs)    Elapsed time: 9.448582s (5.894076s in 131 GCs)   Elapsed time: 2.919532s (1.867242s in 35 GCs)
Elapsed time: 7.156676s (5.486435s in 102 GCs)   Elapsed time: 9.682056s (6.110939s in 136 GCs)   Elapsed time: 3.025861s (1.920943s in 36 GCs)
Elapsed time: 7.398661s (5.650340s in 105 GCs)   Elapsed time: 10.027343s (6.341478s in 141 GCs)  Elapsed time: 3.238696s (1.975818s in 37 GCs)
Elapsed time: 7.751980s (5.920265s in 110 GCs)   Elapsed time: 10.654161s (6.716559s in 146 GCs)  Elapsed time: 3.290497s (2.083083s in 39 GCs)
Elapsed time: 8.152714s (6.133111s in 114 GCs)   Elapsed time: 12.392045s (7.733312s in 152 GCs)  Elapsed time: 3.501953s (2.137579s in 40 GCs)

@tarsius tarsius force-pushed the scalar-quote-performance branch 2 times, most recently from c287c50 to 66206d7 Compare August 9, 2024 20:34
@tarsius tarsius force-pushed the scalar-quote-performance branch from 66206d7 to 5f8aeb2 Compare August 21, 2024 15:16
@tarsius tarsius closed this Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants