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

[Meta] Make SearchResponse use pooled buffers for search hits #102030

Closed
76 of 78 tasks
original-brownbear opened this issue Nov 10, 2023 · 1 comment
Closed
76 of 78 tasks

[Meta] Make SearchResponse use pooled buffers for search hits #102030

original-brownbear opened this issue Nov 10, 2023 · 1 comment
Assignees
@original-brownbear
Labels
Meta :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team

Comments

@original-brownbear
Copy link
Member

original-brownbear commented Nov 10, 2023 •
edited
Loading

WIP issue but lets start collecting related PRs here already:

Test work:

Production work:
@original-brownbear original-brownbear added :Search/Search Search-related issues that do not fall into other categories Meta labels Nov 10, 2023
@original-brownbear original-brownbear self-assigned this Nov 10, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Nov 10, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Nov 10, 2023
@original-brownbear
Fix a few more explicit SearchResponse uses in the server package
1ce52a1 
For elastic#102030
@original-brownbear original-brownbear mentioned this issue Nov 10, 2023
Merged
@benwtrent benwtrent mentioned this issue Nov 10, 2023
Merged
elasticsearchmachine pushed a commit that referenced this issue Nov 10, 2023
@original-brownbear
Fix a few more explicit SearchResponse uses in the server package (#1…
8c19d52 
…02031)

For #102030
@JVerwolf JVerwolf mentioned this issue Nov 10, 2023
Merged
elasticsearchmachine pushed a commit that referenced this issue Nov 10, 2023
@JVerwolf
Update tests to decrement ref count (#102044)
5f08929 
Part of the broader work covered in
#102030

Updates tests in: - HighlighterWithAnalyzersTests.java -
TokenCountFieldMapperIntegrationIT - GeoIpDownloaderIT.java -
DataStreamIT.java
This was referenced Nov 13, 2023
Closed
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Nov 13, 2023
@benwtrent
[Transforms] remove direct reference to SearchResponse in integration…
957510c 
… tests (#102034)

Removes direct reference to SearchResponse in transforms. This was the
only one I could find that was generated from a client call.

All the other ones are created directly or mocks.

In general Transforms does a ton of work while holding a reference to
SearchResponse. We will need to make sure it correct decRefs.

related to: #102030
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this issue Nov 13, 2023
@original-brownbear @davidkyle
Fix a few more explicit SearchResponse uses in the server package (el…
6426cf1 
…astic#102031)

For elastic#102030
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this issue Nov 13, 2023
@JVerwolf @davidkyle
Update tests to decrement ref count (elastic#102044)
0e5d09e 
Part of the broader work covered in
elastic#102030

Updates tests in: - HighlighterWithAnalyzersTests.java -
TokenCountFieldMapperIntegrationIT - GeoIpDownloaderIT.java -
DataStreamIT.java
This was referenced Nov 13, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Nov 14, 2023
@iverase
Remove explicit SearchResponse references from `org.elasticsearch.sea…
3605660 
…rch.nested` (#102117)

relates #102030
This was referenced Dec 19, 2023
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Dec 20, 2023
@original-brownbear
Remove InternalSearchResponse as well as most usage of SearchResponse…
7fc1fcb 
…Sections (#103583)

This indirection isn't really necessary now that its use in the high
level rest client went away. -> flattened the `SearchResponse` class to
make ref counting easier -> removed `InternalSearchResponse` whose
functionality was completely redundant now -> removed serialization and
most of the usage of `SearchResponseSections`, only keeping this for the
few search execution spots that currently use it and where removing it
won't get us anything and will be a huge changeset.

enables the last steps in #102030
@original-brownbear original-brownbear mentioned this issue Dec 20, 2023
Merged
navarone-feekery pushed a commit to navarone-feekery/elasticsearch that referenced this issue Dec 22, 2023
@original-brownbear @navarone-feekery
Fix search response leaks in datastreams and mustache modules (elasti…
04130ad 
…c#103532)

Fixes the remaining test leaks in datastream and mustache. Mustache will
require a separate PR to make the search template response classes
properly ref-counted but we can prepare for that here already.

for elastic#102030
navarone-feekery pushed a commit to navarone-feekery/elasticsearch that referenced this issue Dec 22, 2023
@original-brownbear @navarone-feekery
Fix SearchResponse leaks in REST and Enrich tests (elastic#103533)
6b1f620 
It's in the title, fix a couple of tests here and there. Also, fix REST
tests in particular and add a utility to cleanly read a SearchResponse
from a REST response without leaking the parser.

for elastic#102030
navarone-feekery pushed a commit to navarone-feekery/elasticsearch that referenced this issue Dec 22, 2023
@original-brownbear @navarone-feekery
Remove InternalSearchResponse as well as most usage of SearchResponse…
d4e1d4c 
…Sections (elastic#103583)

This indirection isn't really necessary now that its use in the high
level rest client went away. -> flattened the `SearchResponse` class to
make ref counting easier -> removed `InternalSearchResponse` whose
functionality was completely redundant now -> removed serialization and
most of the usage of `SearchResponseSections`, only keeping this for the
few search execution spots that currently use it and where removing it
won't get us anything and will be a huge changeset.

enables the last steps in elastic#102030
This was referenced Dec 29, 2023
Merged
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Jan 4, 2024
@original-brownbear
Fix incorrect assumptions about BytesReference in StateToProcessWrite…
0bd8ffe 
…rHelper (#103929)

The logic here only works accidentally if the `source` is backed by a
single `byte[]` with offset `0`. Fixed by using the `writeTo` API and
just working on the bytes reference via its index handling to find the
zero byte offset.

Non issue since this isn't causing trouble right now but broke when I
tried using this with sliced buffers for #102030
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jan 5, 2024
@original-brownbear
Fix more search response leaks in tests
51cf4da 
Some more fixes as part of elastic#102030
@original-brownbear original-brownbear mentioned this issue Jan 5, 2024
Merged
original-brownbear added a commit that referenced this issue Jan 5, 2024
@original-brownbear
Fix more search response leaks in tests (#103957)
1a848eb 
Some more fixes as part of #102030
This was referenced Jan 5, 2024
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Jan 5, 2024
@original-brownbear
Make SearchHit have a single primary constructor (#103961)
c8209ae 
Making this class ref-counted is rather complicated due to its
mutability. As a first step towards a clean solution, move to a single
primary constructor and make more fields `final`. Also this slightly
speeds up and saves memory on deserialisation by not having to create
the fields maps twice.

part of #102030
2lambda123 pushed a commit to 2lambda123/elastic-elasticsearch that referenced this issue May 3, 2024
@iverase @gmarouli
Remove explicit SearchResponse references from `org.elasticsearch.sea…
2b543b6 
…rch.suggest` (#102227)

relates elastic/elasticsearch#102030
@trakos trakos mentioned this issue Jun 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@original-brownbear original-brownbear

Labels
Meta :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@original-brownbear @elasticsearchmachine