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

QgsVectorLayerExporter: RAM based logic to decide when to flush features #39440

Merged
merged 3 commits into from
Oct 18, 2020

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Oct 17, 2020

Flush features when we have ~ 10 MB of them in memory.

This is a continuation of #39439 , which requires new core functions: QgsGeometry::wkbSize() and QgsFeature::approximateRAMUsage() . Not sure if this is 3.16 material

@github-actions github-actions bot added this to the 3.16.0 milestone Oct 17, 2020
@rouault rouault force-pushed the feature_ram_estimate branch 2 times, most recently from 55544c6 to 41418a2 Compare October 17, 2020 18:46
@nyalldawson
Copy link
Collaborator

Looks great! Could we make the name "approximateRamUsage" to keep capitalisation consistency?

@rouault
Copy link
Contributor Author

rouault commented Oct 17, 2020

approximateRamUsage looks weird to me, RAM being an acronym. Perhaps approximateMemoryUsage instead ?

@nyalldawson
Copy link
Collaborator

RAM being an acronym

We inherited qt's naming here (e.g. QDomElement), and try to keep consistency with that so that the API is more predictable.

@nyalldawson
Copy link
Collaborator

(but obviously "memory" works too!)

@rouault
Copy link
Contributor Author

rouault commented Oct 17, 2020

ok, renamed to approximateMemoryUsage

@nyalldawson
Copy link
Collaborator

Last thought: 10mb seems rather conservative to me. Would you be happy to bump this up? (A lot of workflows end up with memory layers of a couple of hundred mb, and I've not seen any issues reported regarding this

@rouault
Copy link
Contributor Author

rouault commented Oct 17, 2020

Last thought: 10mb seems rather conservative to me. Would you be happy to bump this up?

Just give me your number :-) But a transaction with 10 MB is already quite large. Not sure that increasing that will improve perfs that much.

@nyalldawson
Copy link
Collaborator

100mb? 50 mb?

@rouault
Copy link
Contributor Author

rouault commented Oct 17, 2020

ok, amended to use 100 MB

@nyalldawson nyalldawson merged commit d191aba into qgis:master Oct 18, 2020
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.

2 participants