Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implemented Snapshot.export #58

Closed
wants to merge 1 commit into from
Closed

Conversation

3y3
Copy link
Member

@3y3 3y3 commented May 1, 2015

For @thlorenz review.

@thlorenz
Copy link
Contributor

thlorenz commented May 1, 2015

Looks nice after quick review and should solve the problem I was trying to tackle in #55.
I'll review this in more detail and give you better feedback by Monday.

@3y3
Copy link
Member Author

3y3 commented May 5, 2015

@thlorenz , Ok, thanks.
@bajtos , any thoughts?

@bajtos
Copy link
Member

bajtos commented May 5, 2015

I'd say the solution is overcomplicated, it should be enough to support the flavour export().pipe(). Everything else can be achieved by stream composition, there is no need to repeat it in v8-profiler.

@3y3
Copy link
Member Author

3y3 commented May 5, 2015

@bajtos , are you propose to return Writable stream if there are no arguments passed to function, or you also propose to completely remove callback strategy for this method?

@bajtos
Copy link
Member

bajtos commented May 5, 2015

are you propose to return Writable stream if there are no arguments passed to function, or you also propose to completely remove callback strategy for this method?

Yes and yes.

See concat-stream for an example of converting a stream-based interface to a callback-based one.

@bajtos
Copy link
Member

bajtos commented May 5, 2015

Code sample:

var callback = function(err, data) { /*...*/ };
var concatStream = require('concat-stream')(function(data) { callback(null, data); });
profiler.takeSnapshot().export()
  .on('error', callback)
  .pipe(concatStream)
  .on('error', callback);

@bajtos
Copy link
Member

bajtos commented May 5, 2015

Also toStream() may be a better method name than export.

@3y3
Copy link
Member Author

3y3 commented May 5, 2015

Also toStream() may be a better method name than export.

I take export name, because I want to implement also import.snapshot method.
Completed API will look like:

var fs = require('fs') ;

profiler.gc().takeSnapshot().export() // We don't need to keep snapshot in memory
  .pipe(fs.createWriteStream('snapshot1'))
  .on('finish', profiler.gc)
  .on('finish', function test() {
    // Do something here.
    //...
    compareHeapState();
  });

function compareHeapState() {
  var currentHeap = profiler.takeSnapshot(),
    startHeap = profiler.import.snapshot('snapshot1');

  return currentHeap.compare(startHeap);
}

Of course we don't have gc method now =)

@3y3 3y3 force-pushed the master branch 3 times, most recently from d8da5e6 to 1a24629 Compare May 12, 2015 21:07
@eljefedelrodeodeljefe
Copy link

@3y3 are you moving forwards with this?

@3y3
Copy link
Member Author

3y3 commented Aug 31, 2015

Landed.
I prefer to keep callback api, because not a good idea to install external dependencie only to have ability to save snapshot.

@3y3 3y3 closed this Aug 31, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants