[dart:io] Adds ProcessInfo.{max,current}Rss. Adds OS::MaxRSS on Fuchsia.

[dart:io] Adds ProcessInfo.{max,current}Rss. Adds OS::MaxRSS on Fuchsia.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/9ce608e89d6b68d84f529fd9dab18f2bc61f5a8e

[zra, 2017-04-17 18:19:43]

Description was changed from

==========
[dart:io] Add OS::MaxRSS on Fuchsia. Add Dart accessor for MaxRSS
==========

to

==========
[dart:io] Adds ProcessInfo.{max,current}Rss. Adds OS::MaxRSS on Fuchsia.
==========

[zra, 2017-04-17 18:20:21]

zra@google.com changed reviewers:
+ rmacnak@google.com

[rmacnak, 2017-04-17 20:09:02]

lgtm

https://codereview.chromium.org/2822943002/diff/60001/runtime/bin/process.h
File runtime/bin/process.h (right):

https://codereview.chromium.org/2822943002/diff/60001/runtime/bin/process.h#n...
runtime/bin/process.h:149: static intptr_t CurrentRSS();
Consider uintptr_t or int64_t. A 32-bit process could easily consume >2GB.

[zra, 2017-04-17 21:19:00]

https://codereview.chromium.org/2822943002/diff/60001/runtime/bin/process.h
File runtime/bin/process.h (right):

https://codereview.chromium.org/2822943002/diff/60001/runtime/bin/process.h#n...
runtime/bin/process.h:149: static intptr_t CurrentRSS();
On 2017/04/17 20:09:02, rmacnak wrote:
> Consider uintptr_t or int64_t. A 32-bit process could easily consume >2GB.

Good point. Done.

[zra, 2017-04-17 21:41:46]

Description was changed from

==========
[dart:io] Adds ProcessInfo.{max,current}Rss. Adds OS::MaxRSS on Fuchsia.
==========

to

==========
[dart:io] Adds ProcessInfo.{max,current}Rss. Adds OS::MaxRSS on Fuchsia.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/9ce608e89d6b68d84f529fd9dab18f2bc61f5a8e
==========

[zra, 2017-04-17 21:41:47]

Committed patchset #10 (id:170001) manually as
9ce608e89d6b68d84f529fd9dab18f2bc61f5a8e (presubmit successful).

[kevmoo, 2017-04-17 22:47:41]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2017-04-17 22:47:42]

DBQ: Changelog update?

[Vyacheslav Egorov (Google), 2017-04-18 16:02:03]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2017-04-18 16:02:04]

DBC

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
runtime/bin/process_patch.dart:172: var result = _maxRss();
This code looks bizarre - why is _maxRss simply not throwing itself?

[zra, 2017-04-18 16:07:18]

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
runtime/bin/process_patch.dart:172: var result = _maxRss();
On 2017/04/18 16:02:04, Vyacheslav Egorov (Google) wrote:
> This code looks bizarre - why is _maxRss simply not throwing itself?

Two reasons:

1. Calling Dart_ThrowException and Dart_PropagateError is dangerous and often
leads to memory leaks especially in the dart:io implementation, so I avoid them.

2. An OSError is not an Error and so doesn't receive any special treatment when
set as the return value of a native.

[Vyacheslav Egorov (Google), 2017-04-18 16:21:17]

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
runtime/bin/process_patch.dart:172: var result = _maxRss();
longjmp and setjmp are dangerous indeed[*] - nevertheless the whole VM is built
on top of them and we can hardly avoid using them. Also both of these are APIs
that we provide to external Dart VM users and extension writers - if we avoid
using them, what does it say about our API quality? 

We should either fix these APIs or avoid them entirely, right now we have a
mixture of styles(**) which is very bad sign:  

$ egrep -e "PropagateError|ThrowException" runtime/bin/process.cc | wc -l
20

(*) if only there was a mechanism that seamlessly integrated with C++ exceptions
and ensured correct destruction of all locally constructed objects when
unwinding the stack... ah, wait, there is such a mechanism.

(**) and style with returning Error is extremely confusing. You spent 30 lines
of code writing something that fits into 4 self-documenting lines!

@patch
class ProcessInfo {
  @patch
  static int get maxRss native "ProcessInfo_MaxRSS";
  @patch
  static int get currentRss native "ProcessInfo_CurrentRSS";
}

[zra, 2017-04-18 16:33:58]

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/2822943002/diff/170001/runtime/bin/process_pa...
runtime/bin/process_patch.dart:172: var result = _maxRss();
On 2017/04/18 16:21:17, Vyacheslav Egorov (Google) wrote:
> longjmp and setjmp are dangerous indeed[*] - nevertheless the whole VM is
built
> on top of them and we can hardly avoid using them. Also both of these are APIs
> that we provide to external Dart VM users and extension writers - if we avoid
> using them, what does it say about our API quality? 
> 
> We should either fix these APIs or avoid them entirely, right now we have a
> mixture of styles(**) which is very bad sign:  
> 
> $ egrep -e "PropagateError|ThrowException" runtime/bin/process.cc | wc -l
> 20
> 
> (*) if only there was a mechanism that seamlessly integrated with C++
exceptions
> and ensured correct destruction of all locally constructed objects when
> unwinding the stack... ah, wait, there is such a mechanism.
> 
> (**) and style with returning Error is extremely confusing. You spent 30 lines
> of code writing something that fits into 4 self-documenting lines!
> 
> @patch
> class ProcessInfo {
>   @patch
>   static int get maxRss native "ProcessInfo_MaxRSS";
>   @patch
>   static int get currentRss native "ProcessInfo_CurrentRSS";
> }
> 

Slava, you make some valid points, but:
1. I don't appreciate your snarky tone.
2. If you don't have resources from your team to devote to code-health in
dart:io, then this discussion won't be productive.