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.

Sign up for GitHub

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

Jump to bottom

Headers.fromEntries: using CaseInsensitiveMap.fromEntries #437

Merged
merged 9 commits into from
Jun 18, 2024
Merged

Headers.fromEntries: using CaseInsensitiveMap.fromEntries #437

merged 9 commits into from
Jun 18, 2024

Conversation

gmpassos
Copy link
Contributor

  • Headers:
    • Added the constructor Headers.fromEntries.
    • Added the internal constructor _fromEntries, which uses CaseInsensitiveMap.fromEntries for optimization.
    • Renamed the internal constructor _ to _from, redirecting it to this._fromEntries.
  • [✅ ] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

@gmpassos
* Headers:
761030d 
  * Optimize constructor `_`: use `CaseInsensitiveMap.fromEntries`.
@gmpassos
* Headers:
8a5ccc0 
  * Added the constructor `Headers.fromEntries`.
  * Added the internal constructor `_fromEntries`, which uses `CaseInsensitiveMap.fromEntries` for optimization.
  * Renamed the internal constructor `_` to `_from`, redirecting it to `this._fromEntries`.
@gmpassos
headers_test.dart
e1eacb3
@gmpassos
http_parser: ^4.1.0
5c19a5a
@gmpassos
Copy link
Contributor Author

FYI: @kevmoo

kevmoo
kevmoo approved these changes Jun 17, 2024
View reviewed changes
Copy link
Member

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

I made some tweaks.

@gmpassos
Copy link
Contributor Author

I made some tweaks.

nice

@github-actions github-actions bot added the infra label Jun 17, 2024
@kevmoo
Copy link
Member

kevmoo commented Jun 17, 2024

@gmpassos – and some more tweaks 😄

@kevmoo
Copy link
Member

kevmoo commented Jun 18, 2024

@gmpassos – do you have benchmarks showing this makes things notably faster?

@kevmoo kevmoo merged commit 2536c15 into dart-lang:master Jun 18, 2024
22 checks passed
@kevmoo
Copy link
Member

kevmoo commented Jun 18, 2024

@gmpassos – we need to roll this through our internal "stuff" before we publish. To make sure nothing breaks. I'll let you know when publish happens!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jun 18, 2024
@devoncarew
[deps] rev async, dartdoc, http_parser, shelf
3279b0f 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/a004370..d7c0cd5):
  d7c0cd5  2024-06-17  Kevin Moore  Drop use of pkg:collection whereNotNull() (dart-lang/async#278)

dartdoc (https://github.com/dart-lang/dartdoc/compare/14d33d3..818d1f5):
  818d1f52  2024-06-17  Sam Rawlins  Minor improvement to the 'documentedWhere' comment (dart-lang/dartdoc#3789)
  39591df5  2024-06-17  Sam Rawlins  Pipe the '--stats' flag through for the 'doc sdk' task (dart-lang/dartdoc#3791)
  6b2ee570  2024-06-17  Sam Rawlins  Use 'named' extension in tests (dart-lang/dartdoc#3790)
  fabc394b  2024-06-17  Sam Rawlins  Add more information to various asserts. (dart-lang/dartdoc#3787)
  64982f89  2024-06-17  Parker Lougheed  Update to package:lints v4 (dart-lang/dartdoc#3785)

http_parser (https://github.com/dart-lang/http_parser/compare/71b4c2c..9bf7bd9):
  9bf7bd9  2024-06-17  Kevin Moore  Prepare release v4.1.0 (dart-lang/http_parser#100)

shelf (https://github.com/dart-lang/shelf/compare/4c54af6..2536c15):
  2536c15  2024-06-17  Graciliano Monteiro Passos  `Headers.fromEntries`: using `CaseInsensitiveMap.fromEntries` (dart-lang/shelf#437)

Change-Id: Iff49c7356534f950147b2c0c087e59414a6fc393
Tested: pkg/vm changes are updating test expectations
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/372101
Commit-Queue: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
@gmpassos
Copy link
Contributor Author

@gmpassos – we need to roll this through our internal "stuff" before we publish. To make sure nothing breaks. I'll let you know when publish happens!

Thanks for the quick response and attention. I will wait for the publication...

@kevmoo
Copy link
Member

kevmoo commented Jun 18, 2024

Looks like @devoncarew landed in the Dart SDK 50 minutes ago. We'll have to wait a few hours to make sure that rolls into our internal...things...before we publish

@gmpassos
Copy link
Contributor Author

gmpassos commented Jun 18, 2024 •
edited
Loading

@gmpassos – do you have benchmarks showing this makes things notably faster?

In my benchmark, I get a 1-2% improvement, but note that you need to have real-world headers (real browsers send many more headers than simple HTTP clients). Additionally, you need a heavy load, as this also affects the GC. This is more significant for slower/smaller servers. On a medium-sized server with low traffic, this won't have much impact.

I'm also looking for a way to avoid computation of all canonicalized keys just to build the headers. This will significantly improve performance, or we could use a specialized cache of canonicalized keys, but this will be a future optimization.

@kevmoo
Copy link
Member

kevmoo commented Jun 21, 2024

shelf published w/ this! @gmpassos !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevmoo kevmoo kevmoo approved these changes

Assignees
No one assigned
Labels
infra package:shelf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gmpassos @kevmoo