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

#485 Pick up locale requested by the client #577

Merged

Conversation

Matasx
Copy link
Collaborator

@Matasx Matasx commented Dec 8, 2024

TODO

  • Documentation
  • Acceptance test
  • Testing

@Matasx Matasx linked an issue Dec 8, 2024 that may be closed by this pull request
Copy link
Owner

@Kaliumhexacyanoferrat Kaliumhexacyanoferrat left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks pretty good - just added some notes.

In general, I have the feeling that the concern will do a lot of allocations. I cannot run dotMemory right now but especially the language parsing and the LINQ queries will allocate a lot of memory - typically the modules should try to do as few allocation as possible (for a simple "hello world" example the whole server will only allocate objects for the request, the response objects and some strings). This might not matter for single requests but if you process hundreds or even thousands per second this will be significant (I guess around 5% slow down with this concern in place). This is also the reason why some of the code of GenHTTP probably looks not as elegant as it could. So for example an optimal solution for the parsing would be a Span<char> based parser that just returns the CultureInfo without allocating new strings - I would not do this in this case as this is way to much code/effort and using a compiled Regex is the most reasonable approach here - but you get the picture :)

Don't know whether you have an IDEA Ultimate license - there is dotMemory in the suite which is super helpful to analyze the allocations. I have a free Open Source license from JetBrains for this project, but I am not sure whether I can extend this for another contributor currently as they are running for a whole year (until May 2025).

Some experience of the previous optimization rounds:

  • Try to pre-compute and cache as much variables as possible
  • Try to avoid string allocations where applicable
  • Replace LINQ queries with plain foreach loops
  • Use ArrayPool to buffer results (you don't have this here)

Modules/GenHTTP.Modules.I18n/Parsers/CultureInfoParser.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/Parsers/LanguageParser.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/Types.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/LocalizationConcernBuilder.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/LocalizationConcernBuilder.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/Types.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/Types.cs Outdated Show resolved Hide resolved
Modules/GenHTTP.Modules.I18n/GenHTTP.Modules.I18n.csproj Outdated Show resolved Hide resolved
@Matasx
Copy link
Collaborator Author

Matasx commented Dec 9, 2024

Thank you. The easy parts are updated. I will now focus on the memory consumption and async support. I don't have jetbrains licence, but the memory profiler bundled with MSVS Pro is also somewhat useful. I'll see what I can do. 😸

- Rename `FromLanguage` to `FromRequest`
- Re-order the methods in the concern to follow the lifecycle and visibility
@Kaliumhexacyanoferrat Kaliumhexacyanoferrat merged commit e97acc1 into main Dec 9, 2024
2 checks passed
@Kaliumhexacyanoferrat Kaliumhexacyanoferrat deleted the 489-pick-up-locale-requested-by-the-client branch December 9, 2024 14:55
@Kaliumhexacyanoferrat
Copy link
Owner

Great feature 🥳

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.

Pick up locale requested by the client
2 participants