Skip to content

fix: avoid vector copies in CheckIfSubtreesAreEqual#27854

Open
lhrios wants to merge 2 commits into
microsoft:mainfrom
lhrios:main
Open

fix: avoid vector copies in CheckIfSubtreesAreEqual#27854
lhrios wants to merge 2 commits into
microsoft:mainfrom
lhrios:main

Conversation

@lhrios
Copy link
Copy Markdown
Contributor

@lhrios lhrios commented Mar 26, 2026

indices is built once and then only read during recursive calls to CheckIfSubtreesAreEqual. However it was passed by value, causing a full copy on every recursive call. Changed to const&.

Data from the profiler:

To collect the following data, a model with a single TreeEnsembleClassifier node (5000 trees and 3.3 million nodes) has been used. The loading time dropped from 18 minutes to about 4 seconds.

After

Screenshot 2026-03-25 at 6 40 25 PM

Before

Screenshot 2026-03-25 at 6 40 40 PM

@lhrios
Copy link
Copy Markdown
Contributor Author

lhrios commented Mar 28, 2026

@xadupre or @cbourjau could you take a look please? 🙏

@lhrios
Copy link
Copy Markdown
Contributor Author

lhrios commented Apr 20, 2026

@xadupre or @cbourjau could you take a look please? 🙏

@xadupre I’d appreciate your feedback. We’re already seeing significant improvements in our production environment from this change, and I think other users could benefit as well.

Copy link
Copy Markdown
Contributor

@cbourjau cbourjau left a comment

Choose a reason for hiding this comment

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

Sorry for missing this! Looks great to me! I'm afraid I don't have commit rights, though. @xadupre ?

@xadupre
Copy link
Copy Markdown
Member

xadupre commented May 6, 2026

@copilot can you merge with main branch

@cbourjau
Copy link
Copy Markdown
Contributor

cbourjau commented May 8, 2026

I guess copilot couldn't ;) @xadupre

@xadupre
Copy link
Copy Markdown
Member

xadupre commented May 12, 2026

@cbourjau can you update your PR, maybe I cannot trigger copilot on others PR or I can create another PR taking your changes to complete this task. Whichever works for me.

@cbourjau
Copy link
Copy Markdown
Contributor

@xadupre this is not my PR; I'm just a very interested bystander/profiteer 🙈

@lhrios
Copy link
Copy Markdown
Contributor Author

lhrios commented May 12, 2026

@xadupre, I'm happy to update it. Let me know if there is anything else I should change.

I think I already gave you permissions: 🤔

Screenshot 2026-05-12 at 10 17 13 AM

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