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

make lt-comp go a bit faster #114

Closed
wants to merge 2 commits into from
Closed

make lt-comp go a bit faster #114

wants to merge 2 commits into from

Conversation

mr-martian
Copy link
Contributor

Compiling the cat monodix was taking about 15 seconds that seemed kind of high to me so I implemented the changes mentioned in #96 and also poked at some other things and managed to get the cat compile time down to about 12.5 seconds.

From profiling it looks like 80% of the runtime of lt-comp is in transducer.minimize() which is inconveniently also the the one step of the process that I don't actually understand.

Copy link
Member

@unhammer unhammer left a comment

Choose a reason for hiding this comment

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

mind filling in the git commit messages a bit with the reasoning? (the current message just makes it seem like you fixed a missing thing in the dtd and made some internal speed optimization, but IIUC it's actually a new feature that you have to apply to your dix files to make them compile faster)

paradigms[current_paradigm].minimize();
} else {
paradigms[current_paradigm].joinFinals();
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be if(current_minimise) minimize(); joinFinals() , ie. we want to joinFinals also when we minimize? (Seems like the current change would also alter how non-paradigms are compiled!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

minimize() calls reverse() which calls joinFinals(), so there shouldn't be a change in behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Calls reverse twice in fact, so it both joins finals and initials … I suppose nowhere in that sequence any new (unjoined) final is added so it's safe then :)

for(auto& it2 : finals) {
finals_state.insert(it2.first);
}

Copy link
Member

Choose a reason for hiding this comment

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

for our edification, can you explain why this block is moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's calculating a list of the final states in the original transducer, but in its original location it was recalculating it on each iteration but it won't have changed.

@unhammer
Copy link
Member

LGTM

mr-martian added a commit that referenced this pull request Jun 15, 2021
@mr-martian
Copy link
Contributor Author

The efficiency tweaks have now been incorporated into #115, so I'm closing this since the #96 piece probably needs more discussion and merging vs redoing are likely to be of comparable effort.

@mr-martian mr-martian closed this Jun 15, 2021
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.

2 participants