Re: Review Request 66420: Windows: Extracted job object code into `os/windows/jobobject.hpp`.

2018-04-05 Thread Akash Gupta 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66420/#review200572
---


Ship it!




Ship It!

- Akash Gupta


On April 4, 2018, 5:46 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66420/
> ---
> 
> (Updated April 4, 2018, 5:46 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The functions written to deal with job objects on Windows had become
> large enough to warrant being refactored into their own file. Also
> was the perfect opportunity to fix formatting issues.
> 
> When including `jobobject.hpp` for `killtree.hpp`, other unnecessary
> headers were removed.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 742bfc44d68d978dd2249ece500d6f64e4d7f02a 
>   3rdparty/stout/include/stout/os/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> ce8bdcd18305ffb758f22a6c2bbc7393675aebdf 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 739ee4da3f09d2a9597d4451e755e77903e9287d 
> 
> 
> Diff: https://reviews.apache.org/r/66420/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66420: Windows: Extracted job object code into `os/windows/jobobject.hpp`.

2018-04-04 Thread Andrew Schwartzmeyer 


> On April 4, 2018, 3:12 p.m., John Kordich wrote:
> > 3rdparty/stout/include/stout/os/windows/jobobject.hpp
> > Lines 139 (patched)
> > 
> >
> > This is interesting. I understand how you're using this template 
> > function to allocate this structure on the stack which you use a 
> > reinterpret_cast on later as a substitute for the 
> > JOBOBJECT_BASIC_PROCESS_ID_LIST.
> > 
> > But is this really worth doing?  Allocation and deallocation would 
> > happen entirely within this function. I imagine the data we need will be 
> > copied out before deallocation during the insert calls below on the 
> > set object, so the only real issue is the extra time associated 
> > with dynamic memory allocation/deallocation.
> > 
> > What's hairy about the size calculations? I imagine it's not that bad, 
> > probably nothing you wouldn't do normally in C :)
> > 
> > If you do end up staying with this structure, is there a reason the 
> > ProcessIdList member is a DWORD and not a ULONG_PTR, as per 
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms684150(v=vs.85).aspx
> >  ?
> > 
> > It's probably the case that on x86_64 that they are the same size, but 
> > why not mirror the structure identically?

> probably nothing you wouldn't do normally in C

Precisely. This isn't C, so why manually allocate and calculate and deallocate 
when we don't need to? It's too easy to screw up.

> is there a reason the ProcessIdList member is a DWORD and not a ULONG_PTR

Yes. The type of the identifer in the Windows struct (because it's the pointer 
to the beginning of an "array") is `ULONG_PTR`, but it is not actually an 
array. The arary itself (which you would have to allocate in the correct 
location for this struct) is an array of `DWORD` (since it's an array of PIDs); 
so when we declare an actual array of PIDs, we declare a `DWORD array[n]`.

The C struct in the Windows API ends with a `ULONG_PTR` because the actual 
defined struct does not contain an array. It instead expects you to allocate a 
contiguous block large enough to hold the struct and the array, and then the 
type in the struct is just the array pointer of type `ULONG_PTR`.

And this is why we do it this why, and not Windows's way. It's too easy to get 
wrong.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66420/#review200502
---


On April 3, 2018, 10:46 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66420/
> ---
> 
> (Updated April 3, 2018, 10:46 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The functions written to deal with job objects on Windows had become
> large enough to warrant being refactored into their own file. Also
> was the perfect opportunity to fix formatting issues.
> 
> When including `jobobject.hpp` for `killtree.hpp`, other unnecessary
> headers were removed.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 742bfc44d68d978dd2249ece500d6f64e4d7f02a 
>   3rdparty/stout/include/stout/os/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> ce8bdcd18305ffb758f22a6c2bbc7393675aebdf 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 739ee4da3f09d2a9597d4451e755e77903e9287d 
> 
> 
> Diff: https://reviews.apache.org/r/66420/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66420: Windows: Extracted job object code into `os/windows/jobobject.hpp`.

2018-04-04 Thread John Kordich via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66420/#review200503
---


Ship it!




Ship It!

- John Kordich


On April 4, 2018, 5:46 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66420/
> ---
> 
> (Updated April 4, 2018, 5:46 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The functions written to deal with job objects on Windows had become
> large enough to warrant being refactored into their own file. Also
> was the perfect opportunity to fix formatting issues.
> 
> When including `jobobject.hpp` for `killtree.hpp`, other unnecessary
> headers were removed.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 742bfc44d68d978dd2249ece500d6f64e4d7f02a 
>   3rdparty/stout/include/stout/os/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> ce8bdcd18305ffb758f22a6c2bbc7393675aebdf 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 739ee4da3f09d2a9597d4451e755e77903e9287d 
> 
> 
> Diff: https://reviews.apache.org/r/66420/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66420: Windows: Extracted job object code into `os/windows/jobobject.hpp`.

2018-04-04 Thread John Kordich via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66420/#review200502
---


Fix it, then Ship it!




Oops. I just realized as I was getting through this code review that it was 
just a code move into jobobject.hpp.  You can read my comment if you'd like. 
But I was going to sign off anyway :)


3rdparty/stout/include/stout/os/windows/jobobject.hpp
Lines 139 (patched)


This is interesting. I understand how you're using this template function 
to allocate this structure on the stack which you use a reinterpret_cast on 
later as a substitute for the JOBOBJECT_BASIC_PROCESS_ID_LIST.

But is this really worth doing?  Allocation and deallocation would happen 
entirely within this function. I imagine the data we need will be copied out 
before deallocation during the insert calls below on the set object, 
so the only real issue is the extra time associated with dynamic memory 
allocation/deallocation.

What's hairy about the size calculations? I imagine it's not that bad, 
probably nothing you wouldn't do normally in C :)

If you do end up staying with this structure, is there a reason the 
ProcessIdList member is a DWORD and not a ULONG_PTR, as per 
https://msdn.microsoft.com/en-us/library/windows/desktop/ms684150(v=vs.85).aspx 
?

It's probably the case that on x86_64 that they are the same size, but why 
not mirror the structure identically?


- John Kordich


On April 4, 2018, 5:46 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66420/
> ---
> 
> (Updated April 4, 2018, 5:46 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The functions written to deal with job objects on Windows had become
> large enough to warrant being refactored into their own file. Also
> was the perfect opportunity to fix formatting issues.
> 
> When including `jobobject.hpp` for `killtree.hpp`, other unnecessary
> headers were removed.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 742bfc44d68d978dd2249ece500d6f64e4d7f02a 
>   3rdparty/stout/include/stout/os/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> ce8bdcd18305ffb758f22a6c2bbc7393675aebdf 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 739ee4da3f09d2a9597d4451e755e77903e9287d 
> 
> 
> Diff: https://reviews.apache.org/r/66420/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>