TTD workaround for fs_stats issue. #194
Conversation
Signed-off-by: Mark Marron <[email protected]>
| // needs room to store data for *two* `fs.Stats` instances. | ||
| fields = new double[2 * 14]; | ||
| Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), | ||
| new double[2 * 14], |
There was a problem hiding this comment.
Who frees this allocation?
There was a problem hiding this comment.
This is not related to @mrkmarron changes but was like this in upstream. I have raised the question to check this.
digitalinfinity
left a comment
digitalinfinity
left a comment
There was a problem hiding this comment.
LGTM modulo that one question. Also, I'm guessing the perf impact of this change should be negligible but it would be good to run the benchmarks in benchmark\fs to verify.
|
@mrkmarron - I get your changes, but instead of modifying node code in so many places, why not have simple change to pass Also, I agree with @digitalinfinity . We should do a perf run for |
|
Landed in 87ce098 since TTD is broken badly without this change |
|
Sure, the main reason for doing minimal changes on node side is to avoid resolving merge conflicts around that code in future. |
|
I agree with the goal of minimal changes and I initially tried the approach we discussed of explicitly passing |
Fix for build break in PR #194. PR-URL: mrkmarron@fafa155
Fixes: nodejs#194 PR-URL: nodejs#199 Reviewed-By: Hitesh Kanwathirtha <[email protected]>
3 participants
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
fs
Description of change
Fix for TTD break after change to FS Stats access.
PR-URL: mrkmarron@3e52041