Skip to content

Conversation

@rmartinc
Copy link

Hi,

Fix for issue #57 that adds a simple class map to speed up class resolution. We are very interested in this because previously we have already seen complains about performance issues when not having a similar cache.

It is a simple solution to start the PR with something. If you think that a more complicated solution is required (like a max size for the map for example) or you see any improvement just let me know. No tests added because I really don't know where the tests for this project are (maybe in another project?). Anyway, anything you want to change or improve just ask for it here.

…atic fields for imported classes

Signed-off-by: rmartinc <rmartinc@redhat.com>
@markt-asf
Copy link
Contributor

As far as this PR goes, I'd prefer to see the cache use Map<String,Class> to avoid the casts.

However, I think there is wider question of where the best place is for this optimisation. I'd lean towards putting it in ImportHandler so the optimisation applies more widely.

@fl4via
Copy link
Contributor

fl4via commented Nov 13, 2019

@markt-asf +1 I agree that ImportHandler makes more sense for the map

@rmartinc
Copy link
Author

@markt-asf @fl4via It's not so easy, The ImportHandler already uses some caches to speed up this. But the problem is that the ImportHandler is created by the ElContext so it's short lived (every request loads all the needed classes again).

My only idea about improving the ImportHandler is passing a class cache to it and that cache is maintained at upper level. But this is an important change.

@fl4via
Copy link
Contributor

fl4via commented Nov 13, 2019

@rmartinc I see. In that case, we can discuss such a change in the spec list? In the meantime, I would go for the fix in this PR, because it is an issue with a performance impact for users of the spec.

@rmartinc
Copy link
Author

After looking to this deeply, I think it makes no sense to fix this at API level. I think that our solution is very wildfly dependent and it is based a lot in how the deployment handling is managed.

So I'm going to close this PR. Sorry for bothering you! And thanks for looking!

@rmartinc rmartinc closed this Nov 14, 2019
@rmartinc rmartinc deleted the issue57 branch November 14, 2019 17:21
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