Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

[ETFeeder] Speedup et_feeder and resolve uninitialized attrs values in ETFeederNode #60

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[ETFeeder] Speedup et_feeder and resolve uninitialized attrs values in ETFeederNode #60

wants to merge 2 commits into from

Conversation

changhai0109
Copy link

@changhai0109 changhai0109 commented Nov 21, 2023 •
edited
Loading

  1. Updated ETFeeder::readNextWindow to speed up by removing some extra resolveDeps() calls.
  2. Assigned default values of attrs in ETFeederNode to avoid uninitialized variables and leading undefined behavior with -O3 compile flag.

@changhai0109 changhai0109 requested a review from a team as a code owner November 21, 2023 16:04
@changhai0109 changhai0109 self-assigned this Nov 21, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 21, 2023 •
edited
Loading

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@srinivas212
Copy link
Contributor

This is a useful fix. ET feeder is being used by other downstream use cases and we need (a) unit test (b) before and after improvements. Thanks!

srinivas212
srinivas212 reviewed Nov 22, 2023
View reviewed changes
et_converter/text2chakra_converter.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we separate the specific changes with this PR and move the formatting changes to another PR?

Copy link
Author

Choose a reason for hiding this comment

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

These format changes are not meant to be there, instead come from the auto fomatter of my IDE.

I cleaned these format changes, also cleaned the commit history. Now it only contains the necessary changes there.

@changhai0109 changhai0109 force-pushed the changhai-speedup-etfeeder branch 2 times, most recently from 9d111ec to 3855955 Compare November 22, 2023 21:45
@changhai0109
Copy link
Author

Moved the commit of changing et_converter output this PR, will submit another one later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@srinivas212 srinivas212 srinivas212 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@changhai0109 changhai0109

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@changhai0109 @srinivas212