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

implement area size heuristic for area:id queries #364

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

implement area size heuristic for area:id queries #364

wants to merge 4 commits into from

Conversation

tyrasd
Copy link
Contributor

@tyrasd tyrasd commented Mar 2, 2017 •
edited
Loading

fixes #359. The implementation feels a bit hacky / inefficient (since we need to iterate twice over all areas now), but I didn't see another way to access the area's used_indices property. Any pointers for possible improvements are welcome.

mmd-osm
mmd-osm reviewed Mar 2, 2017
View reviewed changes
src/overpass_api/statements/area_query.cc
@@ -287,8 +287,20 @@ void Area_Query_Statement::get_ranges

bool Area_Constraint::delivers_data(Resource_Manager& rman)
{
int counter = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

int counter = 0 in line 311 should probably be removed as well, otherwise line 321 won't return the correct value I guess (variable counter gets shadowed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right! forgot to remove that one

mmd-osm
mmd-osm reviewed Mar 2, 2017
View reviewed changes
src/overpass_api/statements/area_query.cc Outdated
it(area_locations_db.flat_begin());
!(it == area_locations_db.flat_end()); ++it)
{
if (area->get_submitted_id() == it.object().id.val())
Copy link
Contributor

@mmd-osm mmd-osm Mar 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can you check if you could use it.handle() ... instead to get the id (avoids some unneeded object instatiation) - not super urgent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean just changing it to it.handle().object().id.val(), right? For me that seems to works as well.

It probably should also be changed on line 244…

Copy link
Contributor

@mmd-osm mmd-osm Mar 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I had it.handle().id() in mind - we have something similar elsewhere but I don't know if this also works in this context without further checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yes, that makes more sense 🙈 -> 4d9992d

Copy link
Contributor

Choose a reason for hiding this comment

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

I think even the .val() at the end is not needed as id() already returns the right type. Can you maybe test this again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I get the following compile-time error (gcc 6.2):

error: no match for ‘operator==’ (operand types are ‘long long int’ and ‘Area_Skeleton::Id_Type {aka Uint32_Index}’

Uint32_Index has the == operator implemented only for comparing with other Uint32_Index values while Area_Query_Statement (for some reason) stores the id it gets in a 64bit integer.

But I think that calling .val() here shouldn't be an issue, since the compiler can inline/optimize that away very easily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fine, let's keep val(). I didn't have the source available when commenting and didn't exactly remember when it was necessary.

tyrasd added a commit to tyrasd/Overpass-API that referenced this pull request Mar 2, 2017
@tyrasd
use iterator.handle() to fetch area ids
ad544de 
see drolbr#364 (comment)
@tyrasd tyrasd changed the title implement area size heuristic for area:id queries, fixes #359 implement area size heuristic for area:id queries Mar 2, 2017
@mmd-osm
Copy link
Contributor

mmd-osm commented Mar 3, 2017 •
edited
Loading

Cherry-picked the changes to my branch, tested both variants. Memory consumption looks good, there's a somewhat larger runtime in one case, which is due to some unrelated code in area_query.cc. Bottom line: result is still way better than before, though.

way["building"](area:3600285864);out;

Command being timed: "./osm3s_query"
User time (seconds): 5.54
System time (seconds): 0.10
Percent of CPU this job got: 99%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.65
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 123124
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 31456
Voluntary context switches: 13
Involuntary context switches: 487
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0

area(3600285864);
way["building"](area._);
out;

Command being timed: "./osm3s_query"
User time (seconds): 3.58
System time (seconds): 0.09
Percent of CPU this job got: 99%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:03.67
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 122904
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 36025
Voluntary context switches: 13
Involuntary context switches: 191
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0

@mmd-osm
Copy link
Contributor

mmd-osm commented Mar 8, 2017 •
edited
Loading

I had some issues to apply this PR cleanly, although Github claims that everything is fine. I think this a side effect of the most recent std:: additions across the whole source code. Can you merge the current master and update the pr? thanks!

@tyrasd tyrasd changed the base branch from minor_issues to master March 8, 2017 12:58
@tyrasd
Copy link
Contributor Author

tyrasd commented Mar 8, 2017

although Github claims that everything is fine

I think that was simply because the PR was requested against the minor_issues branch. I've changed it to master now and am looking into the conflicts.

@tyrasd
Copy link
Contributor Author

tyrasd commented Mar 8, 2017

should be resolved now

@mmd-osm
Copy link
Contributor

mmd-osm commented Mar 28, 2017

fyi: I deployed your patch on the following endpoint: http://dev.overpass-api.de/api_mmd/

@mmd-osm
Copy link
Contributor

mmd-osm commented Apr 11, 2018 •
edited
Loading

This one got superseded by dba448f -> this pull request can be closed, there's nothing left to do.

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

@mmd-osm mmd-osm mmd-osm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

memory-inefficient area:id filter
2 participants
@tyrasd @mmd-osm