You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rukletsov <al...@mesosphere.io> on 2015/03/03 21:11:15 UTC
Review Request 31700: Reserved memory for JSON arrays where
appropriate.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-2353
https://issues.apache.org/jira/browse/MESOS-2353
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/http.cpp 117c0ee720a60a1d8a25359028bad803f1fc2b07
Diff: https://reviews.apache.org/r/31700/diff/
Testing
-------
make check (OS X 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 3, 2015, 8:28 p.m., Dominic Hamon wrote:
> > do you have a benchmark for before/after?
Not yet. I expect this change to be "not worse" than the status quo, so the benchmark is needed to check how much performance have we gained. For more other approaches we definitely need a set of tests.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review75029
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 117c0ee720a60a1d8a25359028bad803f1fc2b07
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review75029
-----------------------------------------------------------
do you have a benchmark for before/after?
- Dominic Hamon
On March 3, 2015, 12:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 12:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 117c0ee720a60a1d8a25359028bad803f1fc2b07
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 11, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 471
> > <https://reviews.apache.org/r/31700/diff/2/?file=888170#file888170line471>
> >
> > Do you know if a move is helpful here or if the compiler is already optimizing this?
> >
> > ```
> > object.values["slaves"] = std::move(array);
> > ```
> >
> > Don't do it in this change, we need to measure and I'm just curious :)
>
> Alexander Rukletsov wrote:
> I believe the compiler won't optimize this, since `array` is an lvalue and may be used after this assignment. One thing how we can try to persuade the compiler to move without writing `std::move()` is to wrap array population in a function that returns an array, this will be a rvalue then and c++11-capable compiler *should* choose the right overload, that takes a rvalue ref: http://www.cplusplus.com/reference/map/map/insert/.
>
> But everything more complex than `.reserve()` should be benchmarked, on all official supported compilers if possible. I'll play with this when I have some free cycles.
>
> Cody Maloney wrote:
> From writing my own JSON code, the std::move here is definitely necessary to do this as quickly and cheaply as possible.
>
> Using std::move here I think is actually the right thing to do, rather than trying to convince the compiler via other means, it's explicitly saying to the reader "This variable you were using earlier, it is having it's contents ripped out of it".
>
> Putting it into a function and guaranteeing NRVO or RVO fire so that you get the "cheap" move insertion has a lot of things people can disturb only slightly and end up breaking it.
>
> Cody Maloney wrote:
> Updating the instance in <stout/protobuf.hpp> where the JSON::Array is copied to be a move would also probably help significantly.
>
> The way JSON::Protobuf's api is setup with Operator() forces a copy of the JSON::Object after it is fully constructed which is rather expensive. Would be good to make JSON::Protobuf() just be a function which can then use the object internally, and move out the result rather than forcing the full object copy.
>
> Michael Park wrote:
> TL;DR: `std::move` will help.
>
> ### Does the compiler see that the `array` variable can be moved? No.
>
> It looks like the compiler could probably perform a NRVO-like optimization here, but it doesn't.
>
> ```cpp
> #include <iostream>
>
> class Foo {
> public:
> Foo() { std::cout << "Foo()" << std::endl; }
>
> Foo(const Foo &) { std::cout << "Foo(const Foo &)" << std::endl; }
>
> Foo(Foo &&) noexcept { std::cout << "Foo(Foo &&)" << std::endl; }
>
> Foo &operator=(const Foo &) {
> std::cout << "Foo &operator=(const Foo &)" << std::endl;
> return *this;
> }
>
> Foo &operator=(Foo &&) noexcept {
> std::cout << "Foo &operator=(Foo &&)" << std::endl;
> return *this;
> }
> };
>
> int main() {
> Foo result;
> {
> Foo x;
> result = x;
> }
> {
> Foo y;
> result = std::move(y);
> }
> }
> ```
>
> The above prints:
>
> ```bash
> Foo()
> Foo()
> Foo &operator=(const Foo &)
> Foo()
> Foo &operator=(Foo &&)
> ```
>
> That is compiled with `clang++ -std=c++14 -O2 a.cc`. It's safe to say that if can't optimize that obvious-looking case, ours won't be optimized either.
The reason why I mentioned convincing the compiler instead of `std::move()`, is because I don't see `std::move()` in https://mesos.apache.org/documentation/latest/mesos-c++-style-guide/
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Michael Park <mc...@gmail.com>.
> On March 11, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 471
> > <https://reviews.apache.org/r/31700/diff/2/?file=888170#file888170line471>
> >
> > Do you know if a move is helpful here or if the compiler is already optimizing this?
> >
> > ```
> > object.values["slaves"] = std::move(array);
> > ```
> >
> > Don't do it in this change, we need to measure and I'm just curious :)
>
> Alexander Rukletsov wrote:
> I believe the compiler won't optimize this, since `array` is an lvalue and may be used after this assignment. One thing how we can try to persuade the compiler to move without writing `std::move()` is to wrap array population in a function that returns an array, this will be a rvalue then and c++11-capable compiler *should* choose the right overload, that takes a rvalue ref: http://www.cplusplus.com/reference/map/map/insert/.
>
> But everything more complex than `.reserve()` should be benchmarked, on all official supported compilers if possible. I'll play with this when I have some free cycles.
>
> Cody Maloney wrote:
> From writing my own JSON code, the std::move here is definitely necessary to do this as quickly and cheaply as possible.
>
> Using std::move here I think is actually the right thing to do, rather than trying to convince the compiler via other means, it's explicitly saying to the reader "This variable you were using earlier, it is having it's contents ripped out of it".
>
> Putting it into a function and guaranteeing NRVO or RVO fire so that you get the "cheap" move insertion has a lot of things people can disturb only slightly and end up breaking it.
>
> Cody Maloney wrote:
> Updating the instance in <stout/protobuf.hpp> where the JSON::Array is copied to be a move would also probably help significantly.
>
> The way JSON::Protobuf's api is setup with Operator() forces a copy of the JSON::Object after it is fully constructed which is rather expensive. Would be good to make JSON::Protobuf() just be a function which can then use the object internally, and move out the result rather than forcing the full object copy.
TL;DR: `std::move` will help.
### Does the compiler see that the `array` variable can be moved? No.
It looks like the compiler could probably perform a NRVO-like optimization here, but it doesn't.
```cpp
#include <iostream>
class Foo {
public:
Foo() { std::cout << "Foo()" << std::endl; }
Foo(const Foo &) { std::cout << "Foo(const Foo &)" << std::endl; }
Foo(Foo &&) noexcept { std::cout << "Foo(Foo &&)" << std::endl; }
Foo &operator=(const Foo &) {
std::cout << "Foo &operator=(const Foo &)" << std::endl;
return *this;
}
Foo &operator=(Foo &&) noexcept {
std::cout << "Foo &operator=(Foo &&)" << std::endl;
return *this;
}
};
int main() {
Foo result;
{
Foo x;
result = x;
}
{
Foo y;
result = std::move(y);
}
}
```
The above prints:
```bash
Foo()
Foo()
Foo &operator=(const Foo &)
Foo()
Foo &operator=(Foo &&)
```
That is compiled with `clang++ -std=c++14 -O2 a.cc`. It's safe to say that if can't optimize that obvious-looking case, ours won't be optimized either.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 11, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 471
> > <https://reviews.apache.org/r/31700/diff/2/?file=888170#file888170line471>
> >
> > Do you know if a move is helpful here or if the compiler is already optimizing this?
> >
> > ```
> > object.values["slaves"] = std::move(array);
> > ```
> >
> > Don't do it in this change, we need to measure and I'm just curious :)
I believe the compiler won't optimize this, since `array` is an lvalue and may be used after this assignment. One thing how we can try to persuade the compiler to move without writing `std::move()` is to wrap array population in a function that returns an array, this will be a rvalue then and c++11-capable compiler *should* choose the right overload, that takes a rvalue ref: http://www.cplusplus.com/reference/map/map/insert/.
But everything more complex than `.reserve()` should be benchmarked, on all official supported compilers if possible. I'll play with this when I have some free cycles.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------
On March 7, 2015, 1:43 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 7, 2015, 1:43 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Cody Maloney <co...@mesosphere.io>.
> On March 11, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 471
> > <https://reviews.apache.org/r/31700/diff/2/?file=888170#file888170line471>
> >
> > Do you know if a move is helpful here or if the compiler is already optimizing this?
> >
> > ```
> > object.values["slaves"] = std::move(array);
> > ```
> >
> > Don't do it in this change, we need to measure and I'm just curious :)
>
> Alexander Rukletsov wrote:
> I believe the compiler won't optimize this, since `array` is an lvalue and may be used after this assignment. One thing how we can try to persuade the compiler to move without writing `std::move()` is to wrap array population in a function that returns an array, this will be a rvalue then and c++11-capable compiler *should* choose the right overload, that takes a rvalue ref: http://www.cplusplus.com/reference/map/map/insert/.
>
> But everything more complex than `.reserve()` should be benchmarked, on all official supported compilers if possible. I'll play with this when I have some free cycles.
>
> Cody Maloney wrote:
> From writing my own JSON code, the std::move here is definitely necessary to do this as quickly and cheaply as possible.
>
> Using std::move here I think is actually the right thing to do, rather than trying to convince the compiler via other means, it's explicitly saying to the reader "This variable you were using earlier, it is having it's contents ripped out of it".
>
> Putting it into a function and guaranteeing NRVO or RVO fire so that you get the "cheap" move insertion has a lot of things people can disturb only slightly and end up breaking it.
Updating the instance in <stout/protobuf.hpp> where the JSON::Array is copied to be a move would also probably help significantly.
The way JSON::Protobuf's api is setup with Operator() forces a copy of the JSON::Object after it is fully constructed which is rather expensive. Would be good to make JSON::Protobuf() just be a function which can then use the object internally, and move out the result rather than forcing the full object copy.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Cody Maloney <co...@mesosphere.io>.
> On March 11, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 471
> > <https://reviews.apache.org/r/31700/diff/2/?file=888170#file888170line471>
> >
> > Do you know if a move is helpful here or if the compiler is already optimizing this?
> >
> > ```
> > object.values["slaves"] = std::move(array);
> > ```
> >
> > Don't do it in this change, we need to measure and I'm just curious :)
>
> Alexander Rukletsov wrote:
> I believe the compiler won't optimize this, since `array` is an lvalue and may be used after this assignment. One thing how we can try to persuade the compiler to move without writing `std::move()` is to wrap array population in a function that returns an array, this will be a rvalue then and c++11-capable compiler *should* choose the right overload, that takes a rvalue ref: http://www.cplusplus.com/reference/map/map/insert/.
>
> But everything more complex than `.reserve()` should be benchmarked, on all official supported compilers if possible. I'll play with this when I have some free cycles.
>From writing my own JSON code, the std::move here is definitely necessary to do this as quickly and cheaply as possible.
Using std::move here I think is actually the right thing to do, rather than trying to convince the compiler via other means, it's explicitly saying to the reader "This variable you were using earlier, it is having it's contents ripped out of it".
Putting it into a function and guaranteeing NRVO or RVO fire so that you get the "cheap" move insertion has a lot of things people can disturb only slightly and end up breaking it.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------
Ship it!
Thanks alex, just one minor item and we can get this committed!
src/common/http.cpp
<https://reviews.apache.org/r/31700/#comment123581>
Can you make these consistent with the ones below?
E.g.
```
{
JSON::Array array;
array.reserve(task.statuses().size()); // MESOS-2353.
foreach (const TaskStatus& status, task.statuses()) {
statuses.values.push_back(model(status));
}
object.values["statuses"] = array;
}
{
JSON::Array array;
array.reserve(task.labels().labels().size()); // MESOS-2353.
foreach (const Label& label, task.labels().labels()) {
labels.values.push_back(JSON::Protobuf(label));
}
object.values["labels"] = array;
}
```
src/master/http.cpp
<https://reviews.apache.org/r/31700/#comment123582>
Do you know if a move is helpful here or if the compiler is already optimizing this?
```
object.values["slaves"] = std::move(array);
```
Don't do it in this change, we need to measure and I'm just curious :)
- Ben Mahler
On March 7, 2015, 1:43 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 7, 2015, 1:43 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76171
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31700]
All tests passed.
- Mesos ReviewBot
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 12, 2015, 7:27 p.m., Ben Mahler wrote:
> > src/common/http.cpp, line 128
> > <https://reviews.apache.org/r/31700/diff/3/?file=891730#file891730line128>
> >
> > Let's end all of these comments with a period. I'll take care of this before committing.
Thanks, Ben; I'll update the style guide then.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76276
-----------------------------------------------------------
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 12, 2015, 7:27 p.m., Ben Mahler wrote:
> > src/common/http.cpp, line 128
> > <https://reviews.apache.org/r/31700/diff/3/?file=891730#file891730line128>
> >
> > Let's end all of these comments with a period. I'll take care of this before committing.
>
> Alexander Rukletsov wrote:
> Thanks, Ben; I'll update the style guide then.
https://reviews.apache.org/r/32001/
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76276
-----------------------------------------------------------
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76276
-----------------------------------------------------------
Ship it!
Thanks Alex! Let's look at using std::move as well if that works on gcc 4.4 (since that's still inside configure.ac).
src/common/http.cpp
<https://reviews.apache.org/r/31700/#comment123796>
Let's end all of these comments with a period. I'll take care of this before committing.
- Ben Mahler
On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:40 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/
-----------------------------------------------------------
(Updated March 11, 2015, 10:40 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed Ben Mahler's comments.
Bugs: MESOS-2353
https://issues.apache.org/jira/browse/MESOS-2353
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24
Diff: https://reviews.apache.org/r/31700/diff/
Testing
-------
make check (OS X 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review75611
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31700]
All tests passed.
- Mesos ReviewBot
On March 7, 2015, 1:43 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 7, 2015, 1:43 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
> src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/
-----------------------------------------------------------
(Updated March 7, 2015, 1:43 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed Ben Mahler's comments.
Bugs: MESOS-2353
https://issues.apache.org/jira/browse/MESOS-2353
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220
Diff: https://reviews.apache.org/r/31700/diff/
Testing
-------
make check (OS X 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review75150
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31699, 31700]
All tests passed.
- Mesos ReviewBot
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 117c0ee720a60a1d8a25359028bad803f1fc2b07
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31700: Reserved memory for JSON arrays where
appropriate.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review75230
-----------------------------------------------------------
src/master/http.cpp
<https://reviews.apache.org/r/31700/#comment122194>
Let's keep the .reserve() optimization very explicit, can you just use `.reserve()` explicitly here and elsewhere? It would be helpful to have `"// MESOS-2353"` as a trailing comment on the reserve calls as well.
src/master/http.cpp
<https://reviews.apache.org/r/31700/#comment122195>
Please revert this one, it's not related to state.json, we're not expecting there to be a lot of frameworks, and copying a pointer is not expensive. This is why benchmarks are nice :)
- Ben Mahler
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 117c0ee720a60a1d8a25359028bad803f1fc2b07
>
> Diff: https://reviews.apache.org/r/31700/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>