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

Fix context.mergeFilters #889

Merged
merged 5 commits into from
Apr 14, 2018
Merged

Conversation

tdesveaux
Copy link
Contributor

Fix related to #884.

This lead to a 10-15% performance drop for me.

@tvandijck
Copy link
Contributor

As much as I agree that this fixes #884, the performance implications are a little too severe... we should really look at ways to do this more conditionally, so the impact is minimized.. So I'm going to mark this as "request changes"...

Copy link
Contributor

@tvandijck tvandijck left a comment

Choose a reason for hiding this comment

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

performance impact too great... lets figure out a way to do this more conditionally.

@tdesveauxPKFX
Copy link
Contributor

@tvandijck I agree with this.
However, if someone can test on another project that would be good.
I tested this in our projects at work that were setup with a highly customized premake4 build and that I "adapted" to work in premake5.

I will still take a look to see what I can do to improve this.

@tdesveauxPKFX
Copy link
Contributor

tdesveauxPKFX commented Apr 12, 2018

With this, performance is no longer an issue, difference with the current implementation in master is marginal.
However, this works only for tags and not other terms that are tables.
As far as I know, this is not an issue for now but it might become one in the future.

Would implementing mergeFilters in c improve performances?

@tvandijck
Copy link
Contributor

I don't know if it would be effective to do it in C... considering you then have to enumerate the table in C, you just end up with a whole lot of calls into lua api's anyway... I think the conditional approach is acceptable, and can always be extended.. I think it's really only the tags that suffer.

@tdesveauxPKFX
Copy link
Contributor

I forgot to mention, I also removed the call to table.joinunique as we don't care about duplicate and it takes more time to check duplicates than just copy the whole thing.

I did not remove the table.joinunique function though, I should remove it before merging this.

@tvandijck tvandijck merged commit 6c7c6c7 into premake:master Apr 14, 2018
@tdesveaux tdesveaux deleted the fix-mergeFilters branch April 16, 2018 13:08
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