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

Memoize the last node in constvalue lists #255

Merged
merged 2 commits into from
Apr 22, 2018

Conversation

Daniel-Cortez
Copy link
Contributor

@Daniel-Cortez Daniel-Cortez commented Jan 13, 2018

The root node in constvalue lists is always treated differently than the others, only its next field is used, so it should be safe to repurpose a part of unused space to memoize the address of the last node in the list. This should speed up the addition of new nodes to the end of the list in append_constval().

@Daniel-Cortez
Copy link
Contributor Author

Tested compiling "modelsizes.inc" (#285)

#include "../include/modelsizes.inc"
#pragma unused MODELS_gColRadius, MODELS_gColOffset, MODELS_gColAABB
main(){}

On my machine the compile time has decreased from ~13.7 to ~0.95 seconds.

@maddinat0r
Copy link
Contributor

Your changes will make the compiler crash if someone in the future decides to actually write something into the name array of a root constvalue node. You should at least add a comment in the code describing the behavior you depend on, and explicitly warn about not writing anything into the root node.

@Daniel-Cortez
Copy link
Contributor Author

@maddinat0r Well yeah, now that I look at the changes I've made in this PR I think the current implementation is pretty hackish. Just adding comments won't really help, it rather needs a complete rewrite and that's exactly what I'm doing as of writing this.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Feb 21, 2018

Ok, so this time instead of just being casted from constvalue to constvalue_root all root nodes are explicitly defined as of type constvalue_root. This way the code should be more readable since now it's easier to distinguish the root nodes from the regular ones.

@Zeex Zeex merged commit 8927443 into pawn-lang:master Apr 22, 2018
@Daniel-Cortez Daniel-Cortez deleted the constvalue-opt branch August 5, 2018 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants