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

refactor(module: select): use Func to get or set value instead of reflection #1168

Merged
merged 4 commits into from
Feb 27, 2021

Conversation

Zonciu
Copy link
Member

@Zonciu Zonciu commented Feb 24, 2021

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • Bundle size optimization
  • Performance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English use Func to get/set value instead of reflection
🇨🇳 Chinese 使用Func代替反射读写数据

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • Changelog is provided or not needed

@anddrzejb
Copy link
Member

@Zonciu I am not a reflection and expression tree expert, so correct me if I am wrong. My understanding after having a very brief look at your code, is that you are still using reflection, but instead of every time, you basically "compile" it on set and when it is used, it is just using compiled code instead of going through reflection again. This is where your performance boost is coming from. So we may loose a bit more time during first rendering, but gain time on every item processing when value/label is required to be extracted to set.

@Zonciu
Copy link
Member Author

Zonciu commented Feb 24, 2021

@anddrzejb You are right, and we can make it go further if it is necessary: caching typeof(TItem) , typeof(TItemValue) , LabelName , DisabledName , GroupName as cache key to reuse those Expression delegates

@anddrzejb
Copy link
Member

Sounds really cool. I have seen reflection quite often used in other components (for example DatePicker, a big performance offender), it would be really nice if we could somehow make the helper you made universal. But this is maybe for some other time. I do not have time unfortunately to checkout your code today and I am leaving for couple of days, so either @ElderJames can review it or I will once I came back after Sunday. Sorry... 😞

Maybe you could propose your caching idea here as well in the meantime?

@Zonciu
Copy link
Member Author

Zonciu commented Feb 24, 2021

I will create a Func cache commit for this.

Actually I have made a helper for property accessing, it works on table component, see Path-based property access

@ElderJames ElderJames enabled auto-merge (squash) February 27, 2021 16:01
@ElderJames ElderJames merged commit fc7a67b into ant-design-blazor:master Feb 27, 2021
zxyao145 pushed a commit to zxyao145/ant-design-blazor that referenced this pull request Mar 20, 2021
…lection (ant-design-blazor#1168)

* refactor(module: select): use Func to get or set value instead of reflection

* refactor(module: select): add delegate cache

* chore: add DelegateCacheKeyComparer

Co-authored-by: James Yeung <shunjiey@hotmail.com>
ElderJames added a commit that referenced this pull request Apr 23, 2022
…lection (#1168)

* refactor(module: select): use Func to get or set value instead of reflection

* refactor(module: select): add delegate cache

* chore: add DelegateCacheKeyComparer

Co-authored-by: James Yeung <shunjiey@hotmail.com>
ElderJames added a commit that referenced this pull request Apr 30, 2022
…lection (#1168)

* refactor(module: select): use Func to get or set value instead of reflection

* refactor(module: select): add delegate cache

* chore: add DelegateCacheKeyComparer

Co-authored-by: James Yeung <shunjiey@hotmail.com>
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.

None yet

3 participants