[engine] Inline execution of Select/Dependencies/Projection/Literal #4331

Merged
merged 7 commits into from Mar 16, 2017

Conversation

Projects
None yet
3 participants
@ity
Member

ity commented Mar 14, 2017

Problem

Our memory consumption has increased in the new engine. One of the reasons has been node creation.

Solution

  • rather than calling context.get(..) directly invoke the run methods or execute the run method bodies
  • For node creation, change self reference to self instance so you can work with instance itself and dont have to clone self while passing it in.

Result

Memory usage went down by about 20% (6.9GB -> 5.5GB).

@ity ity self-assigned this Mar 14, 2017

@ity

This comment has been minimized.

Show comment
Hide comment
@ity

ity Mar 14, 2017

Member

Fix for #4297

Member

ity commented Mar 14, 2017

Fix for #4297

@ity ity requested review from stuhood, kwlzn and peiyuwang Mar 14, 2017

@@ -1 +1 @@
-ae3b1ec1714da0a43486be8dda37cdb8aeca815b
+ea591fc0b0c25f189f18ea8fda225526bd389503

This comment has been minimized.

@ity

ity Mar 14, 2017

Member

will remove this as soon as #4330 goes in

@ity

ity Mar 14, 2017

Member

will remove this as soon as #4330 goes in

This comment has been minimized.

@stuhood

stuhood Mar 14, 2017

Member

Just FYI: these continue to need to be committed: you'll need to merge from master, and re-run your build to commit some 3rd SHA that will be the union of Kris' change and your change.

@stuhood

stuhood Mar 14, 2017

Member

Just FYI: these continue to need to be committed: you'll need to merge from master, and re-run your build to commit some 3rd SHA that will be the union of Kris' change and your change.

This comment has been minimized.

@kwlzn

kwlzn Mar 14, 2017

Member

yeah, would expect this to be branch-specific based on the native engine fingerprint in your branch.

@kwlzn

kwlzn Mar 14, 2017

Member

yeah, would expect this to be branch-specific based on the native engine fingerprint in your branch.

@kwlzn

kwlzn approved these changes Mar 14, 2017

lgtm!

@@ -1 +1 @@
-ae3b1ec1714da0a43486be8dda37cdb8aeca815b
+ea591fc0b0c25f189f18ea8fda225526bd389503

This comment has been minimized.

@kwlzn

kwlzn Mar 14, 2017

Member

yeah, would expect this to be branch-specific based on the native engine fingerprint in your branch.

@kwlzn

kwlzn Mar 14, 2017

Member

yeah, would expect this to be branch-specific based on the native engine fingerprint in your branch.

src/rust/engine/src/nodes.rs
- maybe_drain_handles().map(|handles| {
- externs::drop_handles(handles);
- });
+ if N::is_inline() {

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

Hm... this isn't returning anything... I don't think this will compile?

Also, the indentation in this method is strange.

@stuhood

stuhood Mar 15, 2017

Member

Hm... this isn't returning anything... I don't think this will compile?

Also, the indentation in this method is strange.

This comment has been minimized.

@ity

ity Mar 15, 2017

Member

sorry, this is old -- fixing some conflicts

@ity

ity Mar 15, 2017

Member

sorry, this is old -- fixing some conflicts

@ity

This comment has been minimized.

Show comment
Hide comment
@ity

ity Mar 15, 2017

Member

Just to make reviewing easier, here are the ones that are turned on/off for inlining:

Select
src/rust/engine/src/nodes.rs- return true

SelectLiteral
src/rust/engine/src/nodes.rs- return true

SelectDependencies
src/rust/engine/src/nodes.rs- return true

SelectProjection
src/rust/engine/src/nodes.rs- return true

ReadLink
src/rust/engine/src/nodes.rs- return false

Stat
src/rust/engine/src/nodes.rs- return false

Scandir
src/rust/engine/src/nodes.rs- return false

Snapshot
src/rust/engine/src/nodes.rs- return false

Task
src/rust/engine/src/nodes.rs- return false

NodeKey
src/rust/engine/src/nodes.rs- return true

Member

ity commented Mar 15, 2017

Just to make reviewing easier, here are the ones that are turned on/off for inlining:

Select
src/rust/engine/src/nodes.rs- return true

SelectLiteral
src/rust/engine/src/nodes.rs- return true

SelectDependencies
src/rust/engine/src/nodes.rs- return true

SelectProjection
src/rust/engine/src/nodes.rs- return true

ReadLink
src/rust/engine/src/nodes.rs- return false

Stat
src/rust/engine/src/nodes.rs- return false

Scandir
src/rust/engine/src/nodes.rs- return false

Snapshot
src/rust/engine/src/nodes.rs- return false

Task
src/rust/engine/src/nodes.rs- return false

NodeKey
src/rust/engine/src/nodes.rs- return true

@stuhood

Ok, looks good now. Thanks!

src/rust/engine/src/nodes.rs
@@ -439,7 +446,7 @@ impl Select {
.map(move |snapshot| context.store_snapshot(&snapshot))
.boxed()
]
- } else if self.product() == &context.core.types.files_content {
+ } else if self.product() == &context.core.types.files_content { // this is

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

xx

src/rust/engine/src/nodes.rs
})
.boxed()
}
+
+ fn is_inline() -> bool {
+ return true

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

The return statement is only necessary when you want to return early. In all of these cases, just true/false is fine.

@stuhood

stuhood Mar 15, 2017

Member

The return statement is only necessary when you want to return early. In all of these cases, just true/false is fine.

@ity

This comment has been minimized.

Show comment
Hide comment
@ity

ity Mar 15, 2017

Member

latest visualization:

run 0

Member

ity commented Mar 15, 2017

latest visualization:

run 0

@ity ity merged commit b42157d into pantsbuild:master Mar 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] Inline execution of Select/Dependencies/Projection/Literal (#…
…4331)

### Problem

Our memory consumption has increased in the new engine. One of the reasons has been node creation.

### Solution

- rather than calling context.get(..) directly invoke the run methods or execute the run method bodies
- For node creation, change self reference to self instance so you can work with instance itself and dont have to clone self while passing it in.

### Result
Memory usage went down by about 20% (6.9GB -> 5.5GB).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment