Skip to content

Conversation

vijaykriishna
Copy link

Description

@dweiss
Copy link
Contributor

dweiss commented Oct 13, 2025

PQs are so heavily used in Lucene that any extra cost added to add() will be probably a no-no. Can you run the benchmarks and see how it affects performance?

@vijaykriishna vijaykriishna force-pushed the fix/15309-optimise-pq-remove branch from 39b0a7e to 50bd4bb Compare October 13, 2025 10:21
@benwtrent
Copy link
Member

I don't think any code actually uses remove...I don't think making it faster is worth making anything else more expensive.

@vijaykriishna
Copy link
Author

@dweiss @benwtrent Are you suggesting to get rid of remove function once and for all ?

@vijaykriishna
Copy link
Author

vijaykriishna commented Oct 13, 2025

PriorityQueue.remove function is used by DiversifiedTopDocsCollector. Ref.

.

Hence, felt to not remove it altogether.

@benwtrent
Copy link
Member

@vijaykriishna I would much prefer the single use gets optimized than adding cost to the priority queue to everything.

@dweiss
Copy link
Contributor

dweiss commented Oct 13, 2025

Yes, I think we should remove it. It's only used in one place and this place could probably do away with something else instead.

@dweiss
Copy link
Contributor

dweiss commented Oct 13, 2025

@vijaykriishna if you'd like to continue - feel free to review DiversifiedTopDocsCollector to see if it can be implemented in a different way, then nuke that remove method from the PriorityQueue class! I'll be away this week but it'd be great to push this forward.

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.

3 participants