Conversation
CompactString for IdentifieCompactString for Identifier
CodSpeed Performance ReportMerging #12101 will improve performances by 7.54%Comparing Summary
Benchmarks breakdown
|
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PYI057 | 1 | 0 | 1 | 0 | 0 |
Formatter (stable)
ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)
openai/openai-cookbook (error)
warning: Detected debug build without --no-cache.
error: Failed to parse examples/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression
Formatter (preview)
ℹ️ ecosystem check encountered format errors. (no format changes; 2 project errors)
demisto/content (error)
ruff format --preview --exclude Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
- 'ignore' -> 'lint.ignore'
- 'select' -> 'lint.select'
- 'unfixable' -> 'lint.unfixable'
- 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.
openai/openai-cookbook (error)
ruff format --preview
warning: Detected debug build without --no-cache.
error: Failed to parse examples/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression
954f54b to
dc7cc94
Compare
|
Nice! |
charliermarsh
left a comment
There was a problem hiding this comment.
Nice. I'm a fan of this, and it's a great sign that we can now do it and see such a significant, measurable impact.
|
How did you determine that |
|
Sorry, I should have mentioned this in the PR description. I first started by using I rebased and reopened #12099 for a direct comparison. |
dhruvmanila
left a comment
There was a problem hiding this comment.
This is great! Thanks for doing this.
| pub(crate) fn try_make_suggestion( | ||
| &self, | ||
| name: String, | ||
| name: Name, | ||
| iter: &Expr, |
There was a problem hiding this comment.
Do you think it would be useful to use Into<Name> in the functions like this? That way one can directly pass in String / &str and not worry about constructing the Name. If it's too much work (I guess it is?), it's fine not to do it.
There was a problem hiding this comment.
Not sure. I'm somewhat concerned about accidental monomorphization because some of the methods aren't trivial.
There was a problem hiding this comment.
It's fine to not do it, just wondered if it would be useful.
dc7cc94 to
4ffd07e
Compare
This PR introduces a small string create for
ExprNameandIdentifierto reduce the number of allocations.Namestruct fromred_knot_python_semantictoruff_python_astExprNameandIdentifierto store aNameinstead of aStringNameto useCompactStringwhich shows better performance thansmol_strPerformance improvement
This change should also reduce peak-memory usage.
Why
CompactStringCompactStringshows better performance in the read path thansmol_str. The only disadvantage compared tosmol_stris thatsmol_strsupportsO(1)cloning.I had to update the red-knot symbol table to store references to avoid allocating new strings (it actually already allocated new strings, but we could have removed those allocations when the AST stores.smol_str)