Bug 167054 - Annotate large string tests with largeHeap
Summary: Annotate large string tests with largeHeap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-14 12:33 PST by Yusuke Suzuki
Modified: 2017-01-14 23:23 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.30 KB, patch)
2017-01-14 12:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (10.13 KB, patch)
2017-01-14 23:21 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-01-14 12:33:47 PST
Annotate large string tests with largeHeap
Comment 1 Yusuke Suzuki 2017-01-14 12:34:03 PST
Created attachment 298859 [details]
Patch
Comment 2 Darin Adler 2017-01-14 16:52:05 PST
Comment on attachment 298859 [details]
Patch

Seems fine to me, but I am too ignorant about our testing system to be the reviewer. What problem does this fix?
Comment 3 Filip Pizlo 2017-01-14 17:54:04 PST
(In reply to comment #2)
> Comment on attachment 298859 [details]
> Patch
> 
> Seems fine to me, but I am too ignorant about our testing system to be the
> reviewer. What problem does this fix?

The test will be skipped if you --memory-limited.

The "//@" lines trigger the JSC test harness to eval that ruby statement in the context of run-jsc-stress-tests. I hadn't been aware of the largeHeap function but here it is inside run-jsc-stress-tests:

def largeHeap
    if $memoryLimited
        $didAddRunCommand = true
        puts "Skipping #{$collectionName}/#{$benchmark}"
    end
end

The only thing I would write differently here is that:

        $didAddRunCommand = true
        puts "Skipping #{$collectionName}/#{$benchmark}"

Is just the body of skip, so this should probably have been:

def largeHeap
    skip if $memoryLimited
end

But if you do that, then it's not clear if it's better for tests to say:

//@ largeHeap

or for them to say:

//@ skip if $memoryLimited

I actually somewhat prefer the latter, since it answers Darin's question immediately.  "largeHeap" does not tell me anything about the behavior while "skip if $memoryLimited" is self-explanatory.
Comment 4 Yusuke Suzuki 2017-01-14 22:37:28 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 298859 [details]
> > Patch
> > 
> > Seems fine to me, but I am too ignorant about our testing system to be the
> > reviewer. What problem does this fix?
> 
> The test will be skipped if you --memory-limited.
> 
> The "//@" lines trigger the JSC test harness to eval that ruby statement in
> the context of run-jsc-stress-tests. I hadn't been aware of the largeHeap
> function but here it is inside run-jsc-stress-tests:
> 
> def largeHeap
>     if $memoryLimited
>         $didAddRunCommand = true
>         puts "Skipping #{$collectionName}/#{$benchmark}"
>     end
> end
> 
> The only thing I would write differently here is that:
> 
>         $didAddRunCommand = true
>         puts "Skipping #{$collectionName}/#{$benchmark}"
> 
> Is just the body of skip, so this should probably have been:
> 
> def largeHeap
>     skip if $memoryLimited
> end
> 
> But if you do that, then it's not clear if it's better for tests to say:
> 
> //@ largeHeap
> 
> or for them to say:
> 
> //@ skip if $memoryLimited
> 
> I actually somewhat prefer the latter, since it answers Darin's question
> immediately.  "largeHeap" does not tell me anything about the behavior while
> "skip if $memoryLimited" is self-explanatory.

Sounds fine. So, change `// @ largeHeap` use in JSTest / LayoutTests :)
Comment 5 Yusuke Suzuki 2017-01-14 23:21:03 PST
Created attachment 298890 [details]
Patch for landing
Comment 6 Yusuke Suzuki 2017-01-14 23:23:30 PST
Committed r210775: <http://trac.webkit.org/changeset/210775>