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

[PERF] import/export XLSX #5660

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

laa-odoo
Copy link
Collaborator

@laa-odoo laa-odoo commented Feb 10, 2025

Task: 4563258

@robodoo
Copy link
Collaborator

robodoo commented Feb 10, 2025

Pull request status dashboard

@laa-odoo laa-odoo force-pushed the master-perf-add-reverse-lookup-on-getItemId-laa branch from 58367c8 to 05efb53 Compare February 10, 2025 09:25
@laa-odoo laa-odoo changed the title WIP [PERF] getItemId: Use Cache Instead of deepEquals Feb 10, 2025
@laa-odoo laa-odoo force-pushed the master-perf-add-reverse-lookup-on-getItemId-laa branch 2 times, most recently from 5ace8db to 27c595e Compare February 11, 2025 10:06
@laa-odoo laa-odoo changed the title [PERF] getItemId: Use Cache Instead of deepEquals [PERF] import/export XLSX Feb 11, 2025
During data import/export snapshotting, getItemId is
heavily used, and its reliance on deepEquals for item
comparisons can be very time-consuming with large
datasets.

This commit replaces the deepEquals call with a cache
lookup, using pre-populated stringified representations
of the items. This optimization reduces the processing
time during snapshotting.

Tests on our largest datasets show a 30% improvement
in overall import performance.

Task: 4563258
Only declare the result array when necessary.
Optimize element traversal based on type (array or object).

Tests on our largest datasets show a 3% improvement
in overall import performance.

Task: 4563258
@laa-odoo laa-odoo force-pushed the master-perf-add-reverse-lookup-on-getItemId-laa branch from 27c595e to a54d0e9 Compare February 13, 2025 11:53
…or loop in addRows

Previously, heavy PositionMap constants were declared within the for loop, leading to unnecessary overhead on each iteration.
This change moves their declaration outside the loop to optimize performance.

Task: 4563258
During data export, `pushElement` is heavily used,
and its reliance on deepEquals for property comparisons
can be very time-consuming with large datasets.

This commit replaces the deepEquals call with a cache
lookup, using pre-populated stringified representations
of the properties. This optimization reduces the processing
time during export.

For these last two commits, testing on our largest datasets
shows a 90% improvement in overall export perfomance.

Task: 4563258
@laa-odoo laa-odoo force-pushed the master-perf-add-reverse-lookup-on-getItemId-laa branch from a54d0e9 to 61d1524 Compare February 13, 2025 12:38
result += getCanonicalRepresentation(item[i]);
}
return result + "]";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do something like item.map(getCanonicalRepresentation).join(','), but I'm not sure about the performance

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as you want)

@@ -45,3 +59,29 @@ export function* iterateItemIdsPositions(sheetId: UID, itemIdsByZones: Record<st
}
}
}

function getCanonicalRepresentation(item: any): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use number as the canonical representation of number and have number | string as the reverseLookup map key :)

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