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

PropagateDirectory: Investigate if _subJobs should be pre-allocated (reserve()) or squeeze() #5454

Closed
guruz opened this issue Jan 13, 2017 · 8 comments

Comments

@guruz
Copy link
Contributor

guruz commented Jan 13, 2017

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/40930590-propagatedirectory-investigate-if-_subjobs-should-be-pre-allocated-reserve-or-squeeze?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github).
@ckamm
Copy link
Contributor

ckamm commented Jan 17, 2017

@ogoffart I'd say we can squeeze() _subJobs in directories when we're done with them in OwncloudPropagator::start, do you agree?

@jturcotte
Copy link
Member

I think that this is solved by #5400 as the whole PropagateDirectory job is now destroyed by its parent when it's finished.

@guruz
Copy link
Contributor Author

guruz commented Jan 26, 2017

@jturcotte I mean the pre-running peak. If you squeeze() or reserve() it won't take so much space in memory BEFORE running. (directly after constructing).

@jturcotte @ckamm Agree to reopen?

See @ckamm 's comment above..

@jturcotte
Copy link
Member

I interpret "when we're done with them" as "when the job if finished".

reserve() is to prevent reallocating the vector as you fill it if you know the size already, but we don't know it since we iterate over the vector of SyncFileItem until we hit a different parent directory. It doesn't reduce the used memory.

squeeze() is the other way around, when you're removing objects from the vector, it won't reallocate immediately to avoid memcpying to a new location. What I meant is that it won't bring us anything to do this at the end of the PropagateDirectory job, since the whole job (and its vector) will be destroyed immediately after. We could do squeeze after each subjob is done, but this means that we'll reallocate the whole vector each time, only to save a bit of extra memory for the duration of the PropagateDirectory and not reducing the peak, so I'm not sure if it's worth it.

@jturcotte
Copy link
Member

Ahh you guys mean to remove the extra capacity that append might reserve, yes that might help save a few KBytes.

@guruz
Copy link
Contributor Author

guruz commented Jan 26, 2017

Exactly!

@guruz guruz reopened this Jan 26, 2017
@jturcotte
Copy link
Member

You can only do this at the end of OwncloudPropagator::start after the foreach, so you'll already have created all the subjobs and reached the peak at that point, but could keep things tidy with #5451

@ogoffart ogoffart removed their assignment Mar 31, 2017
@ckamm
Copy link
Contributor

ckamm commented Dec 13, 2017

This has been resolved through #5451. During setup we only build a QVector<SyncFileItemPtr> now. We could squeeze that but it'd be a marginal gain.

@ckamm ckamm closed this as completed Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants