perf(rst): build the export data once instead of three times - #670
Open
dexhunter wants to merge 1 commit into
Open
perf(rst): build the export data once instead of three times#670dexhunter wants to merge 1 commit into
dexhunter wants to merge 1 commit into
Conversation
ReSTFormat read dataset.dict three times for every export: once to test whether the dataset was empty, once to measure the column widths and once to render the rows. Each read repackages the whole dataset, and when headers are set it allocates a dict per row that every caller here throws away again by calling row.values(). Every cell was also converted with to_str twice, once to measure it and once to render it. Package the dataset once via _package(dicts=False), which is what the csv, xls, xlsx and ods formats already use, convert each cell once, and pass the strings down to the width calculation and the table builders. Two smaller changes on the same path: cells that contain no whitespace and already fit their column are returned unchanged by TextWrapper, so build those lines directly; and a cell can never contain a word longer than itself, so skip the split when it cannot raise the running maximum. Output is unchanged, including the existing behaviour where duplicate header names collapse columns in the rendered rows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
ReSTFormat.export_setreadsdataset.dictthree times for a single export:export_setreads it to test whether the dataset is empty_get_column_string_lengthsreads it to measure the column widthsEach read repackages the entire dataset. When headers are set,
_packagealsoallocates one dict per row, and every consumer in
_rst.pyimmediately drops thekeys again by calling
row.values(). Each cell also goes throughto_strtwice,once when its length is measured and once when it is rendered.
This packages the dataset once with
_package(dicts=False), which is what_csv.py,_xls.py,_xlsx.pyand_ods.pyalready use, converts each cellonce, and passes the strings down to both the width calculation and the table
builders.
Two smaller changes on the same path:
TextWrapperunchanged, so that line is built directly. Widths of zero or lessstill go to
TextWrapper, which rejects them._max_word_lenis skippedwhen the cell cannot raise the running maximum.
Numbers
Time for one
Dataset.export('rst')call at stock defaults (no kwargs,MAX_TABLE_WIDTH= 80), CPython 3.12.13, median of 7 runs with a freshlygenerated dataset per run:
The denominator is the
export('rst')call itself._packageruns 3 times perexport before this change and once after, counted by instrumenting
Dataset._packageduring one export. This is CPU on the rst export path. It saysnothing about the other formats or about IO.
Output is unchanged
Checked byte for byte against the current
masterimplementation over 76 cases:empty datasets, single cells, all-empty columns, mixed cell types (int, float,
bool,
None,bytes), unicode, whitespace-padded and tab-containing cells,wrapped and unwrapped first columns,
force_grid,max_table_widthfrom 10 to400, and a randomized matrix of shapes and keyword combinations. All 76 produce
identical bytes.
That includes duplicate header names.
dataset.dictroutes each row throughdict(zip(headers, row)), so a repeated header keeps only the last columncarrying that name while the header row still shows every header. That looked
like something to fix, but it is current behaviour and changing it did not belong
in a performance patch, so this reproduces it exactly. The collapsing column
positions are resolved once instead of rebuilding a dict per row. Happy to raise
it separately if you think it is worth changing.
Registered formatters still apply, because the rows come from
_packageratherthan from
Dataset._data.export_set_as_simple_tableandexport_set_as_grid_tablekeep their existing signatures with the new parameteroptional, so calling them directly with just a dataset still works.
The test suite passes, 183 tests, and
ruffis clean.How I found it
Profiling
export('rst')on a 2,000-row dataset putTextWrapper.wrapat 43% ofthe time, the width pass at 26%,
_max_word_lenat 10% andto_strat 6%. Ithen ran an automated optimization search over
_rst.py, gated on byte-exactoutput and on the repository's own test suite. The trajectory is here:
https://dashboard.weco.ai/share/3_8MTQcLFq67dmDdp-N7s5eZbhhM1mQj
The patch in this PR is a smaller hand-written version of what that search found.
Its best step was a little faster again on my benchmark harness, but it rewrote
most of the module and stopped decoding
bytessubclasses, so I kept thenarrower change.