continuing win32/file.c effort

[Jon]

In hopes of keeping moving forward to refactor Windows specific `File`
optimizations to `win32/file.c`, I've created the `win-file/
ruby_1_9_3` patch branch. It currently contains Hiroshi's mods to the
build process and `fileload-ok`, and an initial port of Luis' fenix
extension.

While the fenix port builds without error, it fails `make test` at

https://github.com/ruby/ruby/blob/ruby_1_9_3/sample/test.rb#L2211-2214

and segfaults at https://github.com/ruby/ruby/blob/ruby_1_9_3/sample/test.rb#L2216

Luis' current fenix extension passes these tests...obviously I've
missed something in the translation ;)

I will have very little time next week for debugging so I've pushed
the initial port for your review and tweaking.

As `win-file/ruby_1_9_3` is a patch branch, feel free to `git push --
force` any required changes to get the tests to pass. Just make sure
you fetch to see if anyone else has already forced before you push. If
the branch has already been forced, simply review the new mods and
rebase (or pop your stash) your necessary commits on top of theirs
before forcing again.

Weird I know, but it keeps us from being slowed down by too many
cleanups of merge conflicts.

Jon

[Jon]

> In hopes of keeping moving forward to refactor Windows specific `File`
> optimizations to `win32/file.c`, I've created the `win-file/
> ruby_1_9_3` patch branch. It currently contains Hiroshi's mods to the
> build process and `fileload-ok`, and an initial port of Luis' fenix
> extension.

Unless you can think of good reasons not to, I'm going to rewrite `win-
file/ruby_1_9_3` to remove the ported (failing) fenix code and delete
https://github.com/thecodeshop/fenix as they don't add value and are
distracting.

Keeping the focus on tweaking `fenix/ruby_1_9_3` as needed and
enhancing Luis' primordial fenix repo makes most sense to me.

Hiroshi: is the `win-file/ruby_1_9_3` the best place to maintain your
build process mods until they're committed to ruby-core `trunk` or
should they live elsewhere?

Jon

[Hiroshi Shirosaki]

> Hiroshi: is the `win-file/ruby_1_9_3` the best place to maintain your
> build process mods until they're committed to ruby-core `trunk` or
> should they live elsewhere?

`win-file/ruby_1_9_3` sounds good for tcs experimental build.
However, we need to make a patch against latest trunk to merge upstream.
Is it `win-file/trunk`?

--
Hiroshi Shirosaki

[Jon]

Yes.

FYI, the only thing I changed in `win-file/ruby_1_9_3` from your original gist was switching from `rb_cloexec_open` back to `open` in `file.c:file_load_ok`

Jon

[Hiroshi Shirosaki]

I created experimental fenix integration patch for win32/file.c.
The patch is against trunk.
https://gist.github.com/1986693

The following fixes:
* change static file_expand_path() to public rb_file_expand_path_internal()
* rename Fenix functions name
* use xmalloc/xfree
* add tweaks for `int abs_mode, VALUE result` arguments.
rb_file_absolute_path() uses `abs_mode = 1`
* fix a test failure since fenix uses ENV["HOMEDRIVE"] etc.

I confirmed make using rubyinstaller.
`make test` is OK.
`make test-all`: `test_short_filename` Failure seems only difference from trunk.

--
Hiroshi Shirosaki

[Jon]

Just downloaded and will try applying trunk patch on top of `win-file/ruby_1_9_3` and look at conflicts.

You and Luis were thinking of completely removing path expansion from Fenix but had WEBrick and cygwin concerns. What's your current thoughts?

Cygwin should not a concern. Cygwin is not a ruby-core "best effort" platform for 1.9.3 http://bugs.ruby-lang.org/projects/ruby-trunk/wiki/SupportedPlatforms and frankly, if someone wants cygwin support, they'll need to step up with resourcing.

IMO, as long as things work on OSX, Ubuntu, Arch, FreeBSD, MinGW, and WinSDK, life is good.

Jon

[Hiroshi Shirosaki]

> You and Luis were thinking of completely removing path expansion from Fenix but had WEBrick and cygwin concerns. What's your current thoughts?
>

I'm thinking to apply a patch like the following to webrick for
`test_short_filename` test.
https://gist.github.com/1859130

More work will be required:
* this patch should work on _WIN32 platform (cygwin is not _WIN32 and
doesn't use fenix)
* Win32API will be deprecated. This should use `dl` or other.
* add else condition like the following would be better.

def long_path_name(name)
if (len = GetLongPathName.call(name, nil, 0)).nonzero?
buf = "\0" * len
buf[0...GetLongPathName.call(name, buf, buf.size)]
+ else
+ name
end

--
Hiroshi Shirosaki

[Hiroshi Shirosaki]

> I created experimental fenix integration patch for win32/file.c.
> The patch is against trunk.

I updated the patch.
https://gist.github.com/2057234

* apply webrick fix and `make test-all` result is fine
* add some Fenix fixes for tainted strings etc.

Here is Fenix bench against patched trunk ruby.
https://gist.github.com/2057307

Patched ruby looks comparable speed, but a little slower than Fenix.
I guess `check_expand_path_args(fname, dname)` in
rb_file_expand_path() might do more expensive things.

https://github.com/thecodeshop/ruby/blob/trunk/file.c#L3248

--
Hiroshi Shirosaki

[Luis Lavena]

It does. If no dname is provided (nil) it get the current directory, which is something we already do.

If you remove that should be good. After all is not required in Fenix.

Sorry for top posting. Sent from mobile.

[Hiroshi Shirosaki]

> If you remove that should be good. After all is not required in Fenix.
>

Thanks for comments. I investigated further.

Difference between patched trunk and Fenix seems processing of
rb_get_path_check().
https://github.com/thecodeshop/ruby/blob/trunk/file.c#L152-L181

I added missing code to Fenix and did bench. Pached trunk bench result
was almost same as Fenix.
https://gist.github.com/2131421

The following calls are added.
rb_safe_level()
insecure_obj_p()
rb_enc_get()
rb_enc_asciicompat()
rb_str_new4()

I think above things should not be removed. Some input checks are done
by there code.
What do you think?

--
Hiroshi Shirosaki

[Luis Lavena]

We shouldn't limit to ASCII, there are paths in windows with more than ASCII and Ruby should be able to use it.

Dir.pwd used to have problems with that, so I think expand_path.

Sorry for top posting. Sent from mobile.

[Hiroshi Shirosaki]

> We shouldn't limit to ASCII, there are paths in windows with more than ASCII
> and Ruby should be able to use it.
>
> Dir.pwd used to have problems with that, so I think expand_path.
>

rb_enc_asciicompat() means not just ASCII. We can use more than ASCII.
For example, CP932 and UTF_8 are ASCII compatible.

>ruby -e "p Encoding::CP932.ascii_compatible?"
true

>ruby -e "p Encoding::UTF_8.ascii_compatible?"
true

>ruby -e "p Encoding::UTF_16BE.ascii_compatible?"
false

--
Hiroshi Shirosaki

[Luis Lavena]

On Tue, Mar 20, 2012 at 11:13 AM, Hiroshi Shirosaki
<h.shi...@gmail.com> wrote:
>> We shouldn't limit to ASCII, there are paths in windows with more than ASCII
>> and Ruby should be able to use it.
>>
>> Dir.pwd used to have problems with that, so I think expand_path.
>>
>
> rb_enc_asciicompat() means not just ASCII. We can use more than ASCII.
> For example, CP932 and UTF_8 are ASCII compatible.
>

Hmn, didn't find any documentation on it before, that is why I
completely ignored it.

You're right, let's keep the same things, except for the one that
check args and set dir if nil.

Thank you.
--
Luis Lavena
AREA 17
-
Perfection in design is achieved not when there is nothing more to add,
but rather when there is nothing more to take away.
Antoine de Saint-Exupéry

[Hiroshi Shirosaki]

> You're right, let's keep the same things, except for the one that
> check args and set dir if nil.
>

There is no need for that one.
check_expand_path_args() does nothing if dir is nil.

https://github.com/thecodeshop/ruby/blob/trunk/file.c#L3237

#define check_expand_path_args(fname, dname) \
(((fname) = rb_get_path(fname)), \
(void)(NIL_P(dname) ? (dname) : ((dname) = rb_get_path(dname))))

Getting the current directory with nil dir is positioned in
file_expand_path(). It is not executed since I replaced
file_expand_path() with Fenix implementation.

https://github.com/thecodeshop/ruby/blob/trunk/file.c#L2985

--
Hiroshi Shirosaki

[Luis Lavena]

Thank you Hiroshi for the explanation.

Can you push these changes to Fenix itself and update the patch?

[Hiroshi Shirosaki]

Hi Luis,

> Can you push these changes to Fenix itself and update the patch?
>

I pushed changes to Fenix.

Ruby trunk@35138 patch. make, make test and make test-all seem fine.
https://gist.github.com/2205269

It would be better if the following will be done.
* add tests from Fenix spec to verify no regression
* cleanups comments

--
Hiroshi Shirosaki

[Luis Lavena]

On Mon, Mar 26, 2012 at 11:50 AM, Hiroshi Shirosaki
<h.shi...@gmail.com> wrote:
> Hi Luis,
>
>> Can you push these changes to Fenix itself and update the patch?
>>
>  I pushed changes to Fenix.
>

Thank you Hiroshi.

> Ruby trunk@35138 patch. make, make test and make test-all seem fine.
> https://gist.github.com/2205269
>
> It would be better if the following will be done.
> * add tests from Fenix spec to verify no regression
> * cleanups comments

I'll work on some specs and refresh the entire code comments next weekend.

Once again, thank you for your effort, really appreciated.

[Hiroshi Shirosaki]

>> It would be better if the following will be done.
>> * add tests from Fenix spec to verify no regression
>> * cleanups comments
>
> I'll work on some specs and refresh the entire code comments next weekend.
>

Thank you.

I noticed `else` statement format fix is needed like this commit.
https://github.com/ruby/ruby/commit/b95b56cc3a5c75cb33830cd649a78c17eb777e38

Updated the patch.
https://gist.github.com/2205269

--
Hiroshi Shirosaki

[Luis Lavena]

On Wed, Mar 28, 2012 at 10:53 AM, Hiroshi Shirosaki
<h.shi...@gmail.com> wrote:
>
> I noticed `else` statement format fix is needed like this commit.
> https://github.com/ruby/ruby/commit/b95b56cc3a5c75cb33830cd649a78c17eb777e38
>

Ah, that is a new one :P

Will keep that in mind. Never liked the Egyptian brackets style :P

[Luis Lavena]

On Wed, Mar 28, 2012 at 5:08 PM, Luis Lavena <luisl...@gmail.com> wrote:
> On Wed, Mar 28, 2012 at 10:53 AM, Hiroshi Shirosaki
> <h.shi...@gmail.com> wrote:
>>
>> I noticed `else` statement format fix is needed like this commit.
>> https://github.com/ruby/ruby/commit/b95b56cc3a5c75cb33830cd649a78c17eb777e38
>>
>
> Ah, that is a new one :P
>
> Will keep that in mind. Never liked the Egyptian brackets style :P
>

Bumping my own response.

Shall we submit this as pull request and a ticket on Ruby-Core?

I've been using TCS build with this patch and had no issue with it
over the past few weeks.

Anyone opposes?

[Jon]

On Wednesday, May 9, 2012 11:49:44 AM UTC-4, Luis Lavena wrote:

On Wed, Mar 28, 2012 at 5:08 PM, Luis Lavena <luisl...@gmail.com> wrote:
> On Wed, Mar 28, 2012 at 10:53 AM, Hiroshi Shirosaki
> <h.shi...@gmail.com> wrote:
>>
>> I noticed `else` statement format fix is needed like this commit.
>> https://github.com/ruby/ruby/commit/b95b56cc3a5c75cb33830cd649a78c17eb777e38
>>
>
> Ah, that is a new one :P
>
> Will keep that in mind. Never liked the Egyptian brackets style :P
>

Bumping my own response.

Shall we submit this as pull request and a ticket on Ruby-Core?

I've been using TCS build with this patch and had no issue with it
over the past few weeks.

Anyone opposes?

I *really* want to see this on 1.9.3 and not have to wait for 2.0.0 almost a year from now.

You and Hiroshi are both ruby-core members and should take the path that you think is best. That said, here's another perspective.

1. Get Hiroshi's `File` and build refactorings backported from trunk to ruby_1_9_3. To my bad, I still have not made this request and explained why it's not a refactoring/new feature request :(
2. Submit the pull request for trunk version of the Fenix mods from `win-file/ruby_1_9_3`
3. If/when (2) is accepted, make a ruby_1_9_3 backport request

This may seem convoluted, but it may give us a higher probability of getting Fenix sooner on ruby_1_9_3.

Whichever way you guys decide to go, we need to make sure that any `File` backport requests are *not* viewed as a new features or a refactoring since new features/refactoriings aren't typically backported.

If you think about it, the `File` updates are really just a continuation of the work that already exists in the `win32` subdir. The `File` updates make the code more robust, more maintainable and have the wonderful side effect of amazing performance improvements for Windows users.

Jon

[Hiroshi Shirosaki]

>> Shall we submit this as pull request and a ticket on Ruby-Core?
>>
>> I've been using TCS build with this patch and had no issue with it
>> over the past few weeks.
>>
>> Anyone opposes?
>

I also have no issues with TCS build so far. I'm all for that.

> I *really* want to see this on 1.9.3 and not have to wait for 2.0.0 almost a year from now.

Bug #3924 `Performance bug (in require?)` is considered as a bug and
not fixed yet.
http://bugs.ruby-lang.org/issues/3924

Many Windows users complain so slow on Windows than on Linux. Would
1.9.3 backport be possible for the point?

Comitters are worried about regressions since big change is difficult
to review and tends to cause other bugs.
Maybe we should provide evidences backing no severe issues.

* add unit tests
* testing by how many users
etc.

--
Hiroshi Shirosaki

[Jon]

> >> Shall we submit this as pull request and a ticket on Ruby-Core?
> >>
> >> I've been using TCS build with this patch and had no issue with it
> >> over the past few weeks.
> >>
> >> Anyone opposes?
> >
>
> I also have no issues with TCS build so far. I'm all for that.

Same here, no issues so far.

> > I *really* want to see this on 1.9.3 and not have to wait for 2.0.0 almost a year from now.
>
> Bug #3924 `Performance bug (in require?)` is considered as a bug and not fixed yet.
> http://bugs.ruby-lang.org/issues/3924
>
> Many Windows users complain so slow on Windows than on Linux. Would
> 1.9.3 backport be possible for the point?

Good point. From that perspective the updates are also a fix to a long standing usability bug on Windows.

I advise caution so as to not potentially antagonize other ruby-core committers on this point (last time I did, Matz claimed I was complaining) but maybe we include the following link. Scroll down to the "Linux vs. Windows" section, the boxcharts, and the quote "...the myth [Windows is slower than Linux] is confirmed."

http://programmingzen.com/2010/07/19/the-great-ruby-shootout-july-2010/

> Comitters are worried about regressions since big change is difficult
> to review and tends to cause other bugs.
> Maybe we should provide evidences backing no severe issues.
>
> * add unit tests
> * testing by how many users etc.

Great idea. I'm up for helping write unit tests.

We can also legitimately talk about the number of TCS releases that have included the improvements. I'll work on the wording tomorrow and reply with a suggestion.

Jon

[Jon]

> > > I *really* want to see this on 1.9.3 and not have to wait for 2.0.0 almost a year from now.
> >
> > Bug #3924 `Performance bug (in require?)` is considered as a bug and not fixed yet.
> > http://bugs.ruby-lang.org/issues/3924
> >
> > Many Windows users complain so slow on Windows than on Linux. Would
> > 1.9.3 backport be possible for the point?
>
> Good point. From that perspective the updates are also a fix to a long standing usability bug on Windows.
>
> I advise caution so as to not potentially antagonize other ruby-core committers on this point (last time I did, Matz claimed I was complaining) but maybe we include the following link. Scroll down to the "Linux vs. Windows" section, the boxcharts, and the quote "...the myth [Windows is slower than Linux] is confirmed."
>
> http://programmingzen.com/2010/07/19/the-great-ruby-shootout-july-2010/
>
>
> > Comitters are worried about regressions since big change is difficult
> > to review and tends to cause other bugs.
> > Maybe we should provide evidences backing no severe issues.
> >
> > * add unit tests
> > * testing by how many users etc.
>
> Great idea. I'm up for helping write unit tests.

What specific unit tests should be written in order to enhance the existing ruby-core tests?

> We can also legitimately talk about the number of TCS releases that have included the improvements. I'll work on the wording tomorrow and reply with a suggestion.

Starting in November 2011 with the TCS 1.9.3p0 release, we been refining and testing the Windows `File` performance improvements in the course of the last (7) TCS releases. To date, we've received no reported issues with the improvements.

If you think we need to create a quick blog summarizing results, etc we can jointly edit a post to http://thecodeshop.github.com/ruby/

It's almost done and the only (technical) reason I haven't completed http://thecodeshop.github.com/ruby/2012/03/03/faster-ruby-requires/ is that to get code snippet hilighting like http://thecodeshop.github.com/ruby/2012/02/27/fake-post-2/ you currently have to wrap the code snippet in the markdown template with blocks like:

<pre><code class='language-c'>
<% h do %>
... code snippet ..
<% end %>
</code></pre>

That's OK if we need a quick post to help get `File` optimizations on trunk and ruby_1_9_3. I need to swing back and get a nanoc template helper working so its not that ugly.

Let me know and I'll set up a `gh-pages-review` branch, create some initial content, and each of us can add content for a final published version.

Jon

[Hiroshi Shirosaki]

> What specific unit tests should be written in order to enhance the existing ruby-core tests?

Fenix spec has a lot of tests which are not included in ruby-core test-all.
https://github.com/luislavena/fenix/blob/master/spec/fenix/file_spec.rb

Fenix spec might not sufficient for UNC path, tainted string and
non-ascii encoding path expansion, etc.

JRuby also have a lot of tests for expand_path, though I have not
tried those with MRI.
https://github.com/jruby/jruby/blob/master/test/test_file.rb
test_expand_path_windows
test_expand_path_cross_platform
test_expand_path_nil

It might be better that tests for Windows are added into
test_expand_path in test/ruby/test_file_exhaustive.rb.

>
>> We can also legitimately talk about the number of TCS releases that have included the improvements. I'll work on the wording tomorrow and reply with a suggestion.
>
> Starting in November 2011 with the TCS 1.9.3p0 release, we been refining and testing the Windows `File` performance improvements in the course of the last (7) TCS releases. To date, we've received no reported issues with the improvements.
>
> If you think we need to create a quick blog summarizing results, etc we can jointly edit a post to http://thecodeshop.github.com/ruby/
>

I think posting summarizing results to ruby-core would be nice. Blog is also ok.

--
Hiroshi Shirosaki

[Jon]

> > What specific unit tests should be written in order to enhance the existing ruby-core tests?
>
> Fenix spec has a lot of tests which are not included in ruby-core test-all.
> https://github.com/luislavena/fenix/blob/master/spec/fenix/file_spec.rb
>
> Fenix spec might not sufficient for UNC path, tainted string and
> non-ascii encoding path expansion, etc.
>
> JRuby also have a lot of tests for expand_path, though I have not
> tried those with MRI.
> https://github.com/jruby/jruby/blob/master/test/test_file.rb
> test_expand_path_windows
> test_expand_path_cross_platform
> test_expand_path_nil
>
> It might be better that tests for Windows are added into
> test_expand_path in test/ruby/test_file_exhaustive.rb.

OK, I'll try to find some time to look at those.

Have either of you started adding tests? If yes, which specific tests and where do you plan to push them...fenix repo or the `win-file/ruby_1_9_3` branch?

> >> We can also legitimately talk about the number of TCS releases that have included the improvements. I'll work on the wording tomorrow and reply with a suggestion.
> >
> > Starting in November 2011 with the TCS 1.9.3p0 release, we been refining and testing the Windows `File` performance improvements in the course of the last (7) TCS releases. To date, we've received no reported issues with the improvements.
> >
> > If you think we need to create a quick blog summarizing results, etc we can jointly edit a post to http://thecodeshop.github.com/ruby/
> >
>
> I think posting summarizing results to ruby-core would be nice. Blog is also ok.

Agree.

I pushed a `gh-pages-preview` branch and a `_site_src/content/posts/2012-05-15-faster-windows-ruby-requires.md` example post (markdown) we three can collaborate to finish.

Here's the quickstart...

gem i nanoc adsf
git checkout -b gh-pages-preview tcs/gh-pages-preview
cd _site_src
vim content/posts/_site_src/content/posts/2012-05-15-faster-windows-ruby-requires.md
# add markdown until you're sick of it ;)
nanoc co # compile the site
nanoc view -p 8080
# browse to http://localhost:8080/

Here's the tricky part.

Nanoc compiles a bunch of stuff to the root dir...`css/, 2012/, images/` and mods `index.html`. You only want to commit changes to the `.md` file and none of the others. The mod to `index.html` is the tricky one in that it will be accidently commited with a `git commit -av`...watch out if you're bleary eyed ;)

Once we're happy with the post and I've finished the overall site tweaks, I'll push the static site and it will be available at http://thecodeshop.github.com/ruby/

Jon

---
Fail fast. Fail often. Fail publicly. Learn. Adapt. Repeat.
http://thecodeshop.github.com | http://jonforums.github.com/
twitter: @jonforums

[Hiroshi Shirosaki]

> Have either of you started adding tests?  If yes, which specific tests and where do you plan to push them...fenix repo or the `win-file/ruby_1_9_3` branch?
>

I've not started. `win-file/ruby_1_9_3` or `win-file/trunk` branch
would be better because fenix repo uses spec style while MRI uses
assert_* style.

> I pushed a `gh-pages-preview` branch and a `_site_src/content/posts/2012-05-15-faster-windows-ruby-requires.md` example post (markdown) we three can collaborate to finish.
>

I wrote some text for that. Can you please check it?
https://gist.github.com/2712204


--
Hiroshi Shirosaki

[Jon]

> > Have either of you started adding tests?  If yes, which specific tests and where do you plan to push them...fenix repo or the `win-file/ruby_1_9_3` branch?
> >
>
> I've not started. `win-file/ruby_1_9_3` or `win-file/trunk` branch
> would be better because fenix repo uses spec style while MRI uses
> assert_* style.

OK. Let's start a `win-file/trunk` branch and begin forward-porting the relevant `win-file/ruby_1_9_3` commits and start adding assert_* style tests.


*** READ_ME_NOW...NO REALLY...READ_ME_NOW ****

Anyone following this thread who'd like to contribute by adding tests, send a pull request and if Luis, Hiroshi or I accept it, I'll give you commit access.

And as if that isn't enough enticement, I'll add you to the upcoming "Faster Ruby on Windows" TCS blog post and we'll tweet THANK YOU's with your name as one of the elite guard helping to make Ruby on Windows faster.

Ooooh....that's SOOOO much cooler than cash, right!? ;)

> > I pushed a `gh-pages-preview` branch and a `_site_src/content/posts/2012-05-15-faster-windows-ruby-requires.md` example post (markdown) we three can collaborate to finish.
> >
>
> I wrote some text for that. Can you please check it?
> https://gist.github.com/2712204

Nice. I added, tweaked, added a bit more, and committed.

Jon

[Luis Lavena]

On Thu, May 17, 2012 at 12:11 PM, Jon <jon.f...@gmail.com> wrote:
>
> *** READ_ME_NOW...NO REALLY...READ_ME_NOW ****
>
> Anyone following this thread who'd like to contribute by adding tests, send a pull request and if Luis, Hiroshi or I accept it, I'll give you commit access.
>

I'm a bit off dealing with personal and family matters right now, but
I think converting the tests to MiniTest (not spec) can improve Ruby
own test base.

[Jon]

Luis, Hiroshi,

I want to understand your latest thinking re: submitting integrated Fenix patches to ruby-core. And, I want to lock down a few specific next steps we can quickly finish.

We've discussed adding additional tests and creating a performance summary blog post, all of which are good ideas. As we know, those will take time to complete and, perhaps, not all of them are really needed.

Assuming no surprises, I plan to release tcs-ruby 1.9.3p231 tomorrow on this list like I normally do.

Also, given how well things are working, I'm considering pushing an "official" ANN on Tues/Wed to ruby-talk summarizing all of the current Windows and cross platform performance opts, and a brief summary of the TCS goals so that no one mistakenly believes tcs-ruby is a stand-alone ruby impl. People should view tcs-ruby as a proving ground and staging area for awesomeness that (hopefully) finds its way back into mainstream MRI releases. The goals of the ruby-talk ANN are (a) increase awareness of the opts, and (b) get additional test feedback all with the purpose of increase support to get the patches applied to both trunk and ruby_1_9_3.

Specific thoughts an how best to quickly finish the Fenix effort?

Jon

[Luis Lavena]

Starting this week I'm going to convert all the specs into tests (with MiniTest)

After that, I'm no longer going to work on Fenix standarlone but
instead a branch of ruby/ruby integrating those changes.

I'm writing down the rationale and details to submit that branch to
ruby-core, will send it for proof reading next weekend.

[Jon]

Fabulous on all fronts.

The tcs-ruby ANN next Tues/Wed will be good early support for what you've got planned.

[Hiroshi Shirosaki]

I tested fenix integration further and found an issue with encoding.
Ruby crashes with encoding CP51932/EUC-JP.

$ ruby -ve 'p File.expand_path "\u3042".encode("CP51932")'
tcs-ruby 1.9.3p179 (2012-04-11, TCS patched 2012-04-11) [i386-mingw32]
(crash)

It seems WideCharToMultiByte() doesn't work with CP51932. EUC-JP is
not popular encoding on Windows.

I've updated the patch using ruby API only for such encoding, though
it might not so efficient way.
I confirmed make test && make test-all and fenix specs. It seems fine.
So I've pushed to TCS.

https://github.com/thecodeshop/ruby/commits/win-file/trunk

Ruby 2.0 was changed to preserve path encoding, so Fenix would need
proper way to treat encoding.

--
Hiroshi Shirosaki

[Luis Lavena]

On Fri, Jun 1, 2012 at 12:43 PM, Hiroshi Shirosaki
<h.shi...@gmail.com> wrote:
> I tested fenix integration further and found an issue with encoding.
> Ruby crashes with encoding CP51932/EUC-JP.
>
> $ ruby -ve 'p File.expand_path "\u3042".encode("CP51932")'
> tcs-ruby 1.9.3p179 (2012-04-11, TCS patched 2012-04-11) [i386-mingw32]
> (crash)
>
> It seems WideCharToMultiByte() doesn't work with CP51932. EUC-JP is
> not popular encoding on Windows.
>

Can we add a spec for this to Fenix?

https://github.com/luislavena/fenix

I don't remember there was a test for this encoding on Ruby itself.

> https://github.com/thecodeshop/ruby/commits/win-file/trunk
>
> Ruby 2.0 was changed to preserve path encoding, so Fenix would need
> proper way to treat encoding.
>

I'm converting all the Fenix specs into tests and will create a branch
from Ruby trunk later this weekend,

If Ruby doesn't have a test for this corner case, let's add it.

Thank you.

[Hiroshi Shirosaki]

> Can we add a spec for this to Fenix?
>
> https://github.com/luislavena/fenix

OK. I'll do it later.

>
> I don't remember there was a test for this encoding on Ruby itself.
>
>> https://github.com/thecodeshop/ruby/commits/win-file/trunk
>>
>> Ruby 2.0 was changed to preserve path encoding, so Fenix would need
>> proper way to treat encoding.
>>
>
> I'm converting all the Fenix specs into tests and will create a branch
> from Ruby trunk later this weekend,
>
> If Ruby doesn't have a test for this corner case, let's add it.
>

I've added tests for that as below commit. Please check it.

https://github.com/thecodeshop/ruby/commit/da0dacb18ef2e3a5efcd7117018cf44130031e61#diff-5

Thank you.
--
Hiroshi Shirosaki

[Luis Lavena]

On Sun, Jun 3, 2012 at 9:18 PM, Hiroshi Shirosaki <h.shi...@gmail.com> wrote:
>
> I've added tests for that as below commit. Please check it.
>
> https://github.com/thecodeshop/ruby/commit/da0dacb18ef2e3a5efcd7117018cf44130031e61#diff-5
>

Thank you Hiroshi.

I've been thinking on the corner cases you mentioned before (CP51932/EUC-JP)

What are your thoughts about using Ruby's encoding functionality
within C to perform the change of encoding and cast RSTRING_PTR to
wchar_t to comply with Wide functions?

To my bad, I've been sick and a bit overwhelm with work the past
weeks, so didn't had time to submit this to Ruby-Core (nobu/usa) for
review.

What are your thoughts on you pushing this forward?

[Hiroshi Shirosaki]

> I've been thinking on the corner cases you mentioned before (CP51932/EUC-JP)
>
> What are your thoughts about using Ruby's encoding functionality
> within C to perform the change of encoding and cast RSTRING_PTR to
> wchar_t to comply with Wide functions?
>

Indeed Ruby has functions to encode string to wchar_t (UTF-16LE).
https://github.com/thecodeshop/ruby/blob/trunk/win32/win32.c#L6126

However, expand_path needs to work with miniruby which doesn't have
UTF-16LE encoding. Moreover load `utf_16le.so` is required to use
UTF-16LE, and `load` needs expand_path. It causes recursive calls of
expand_path. So I gave up using Ruby's encoding functionality to
UTF-16LE.

Ruby's original expand_path uses ruby function to encode to UTF-8 and
uses Wide functions against UTF-8.
UTF-8 is integrated to miniruby, so it's free from above loading problem.
https://github.com/thecodeshop/ruby/blob/trunk/file.c#L3176

I uses this approach in Fenix expand_path. This looks not efficient,
but CP51932/EUC-JP is little-used encoding on Windows. So I believe
it's not a severe problem if it passes ruby's test-all.


--
Hiroshi Shirosaki

[Hiroshi Shirosaki]

Update win-file patch

I've added remaining tests from fenix specs and added minor fixes.
I confirmed test-all on win7 and osx.

https://github.com/thecodeshop/ruby/commits/win-file/trunk

--
Hiroshi Shirosaki

[Jon]

> Update win-file patch
>
> I've added remaining tests from fenix specs and added minor fixes.
> I confirmed test-all on win7 and osx.
>
> https://github.com/thecodeshop/ruby/commits/win-file/trunk

I'm a bit out-of-date on Ruby due to work, but have either of you submitted this as a feature request for inclusion in the upcoming 2.0.0?

If it's not yet a feature request, what's left to do before you feel comfortable submitting as a new feature request?

We should split up the tasks and resolve as this is a really important performance/usability fix for 2.0.0 and 1.9.3.

[Luis Lavena]

On Thu, Aug 2, 2012 at 2:55 PM, Jon <jon.f...@gmail.com> wrote:
>> Update win-file patch
>>
>> I've added remaining tests from fenix specs and added minor fixes.
>> I confirmed test-all on win7 and osx.
>>
>> https://github.com/thecodeshop/ruby/commits/win-file/trunk
>
> I'm a bit out-of-date on Ruby due to work, but have either of you submitted this as a feature request for inclusion in the upcoming 2.0.0?
>

I'm writing right now the background of the request and collecting the
last/updated bunch of benchmarks for it.

> If it's not yet a feature request, what's left to do before you feel comfortable submitting as a new feature request?
>

I don't think there is anything else.

> We should split up the tasks and resolve as this is a really important performance/usability fix for 2.0.0 and 1.9.3.
>

I don't think this will be included in 1.9.3 (since it changes
features) but I'll push forward and see how it goes.

Will send an update over the weekend, when I find the time to finish this.

[Luis Lavena]

On Thu, Aug 2, 2012 at 3:48 PM, Luis Lavena <luisl...@gmail.com> wrote:
> On Thu, Aug 2, 2012 at 2:55 PM, Jon <jon.f...@gmail.com> wrote:
>
>> If it's not yet a feature request, what's left to do before you feel comfortable submitting as a new feature request?
>>
>
> I don't think there is anything else.
>

Scratch that, if someone can test with Visual Studio build of Ruby,
will be much appreciated, that way we can confirm it works :)

[Jon]

> >> Update win-file patch
> >>
> >> I've added remaining tests from fenix specs and added minor fixes.
> >> I confirmed test-all on win7 and osx.
> >>
> >> https://github.com/thecodeshop/ruby/commits/win-file/trunk
> >
> > I'm a bit out-of-date on Ruby due to work, but have either of you submitted this as a feature request for inclusion in the upcoming 2.0.0?
> >
>
> I'm writing right now the background of the request and collecting the
> last/updated bunch of benchmarks for it.
>
> > If it's not yet a feature request, what's left to do before you feel comfortable submitting as a new feature request?
> >
>
> I don't think there is anything else.

OK, but if there are boring tasks (eg - testing/building on specific platforms, blog post, show code doesn't break *nix/OSX, etc) that should be done in order to help show ruby-core the compelling benefits and minimal risks, please list specific tasks and ask for crowdsourcing volunteers.

> > We should split up the tasks and resolve as this is a really important performance/usability fix for 2.0.0 and 1.9.3.
> >
>
> I don't think this will be included in 1.9.3 (since it changes
> features) but I'll push forward and see how it goes.

re: 1.9.3, odds are that you're correct. However, we should frame the patch as a platform-specific usability fix rather than a new feature.

As you are very aware, there are plenty of places in the existing codebase that contain platform-specific implementations. The Fenix `File` improvements simply correct the existing Win32 implementation using a much more maintainable (and developer friendly) modular implementation. As an added benefit, Windows users now see a substantial performance improvement with 1.9.3 and 2.0.0. But the performance benefits are really a (fantastic) side-effect of properly coding the current `File` implementation.

Fenix `File` fixes are conceptually the same as Hiroshi's recent 1.9.3 IO fixes that have made it into trunk and have been backported to 1.9.3. They are both platform-specific fixes to performance problems that had been allowed to linger until they became real usability problems for MRI Windows users.

They are not new features, they are usability fixes important to both the current 1.9.x family and the yet-to-be-released 2.0.0 version.

The above "framing" could use a bit more refining, so for others who also feel strongly about this issue, jump in with feedback as we have one chance to get it right.

[Jon]

> On Thu, Aug 2, 2012 at 3:48 PM, Luis Lavena <luisl...@gmail.com> wrote:
> > On Thu, Aug 2, 2012 at 2:55 PM, Jon <jon.f...@gmail.com> wrote:
> >
> >> If it's not yet a feature request, what's left to do before you feel comfortable submitting as a new feature request?
> >>
> >
> > I don't think there is anything else.
> >
>
> Scratch that, if someone can test with Visual Studio build of Ruby,
> will be much appreciated, that way we can confirm it works :)

FYI, as part of the 1.9.3 tcs-ruby releases, I build and test (`make test`) using Windows SDK 7.1 32bit similar to the "Building with Windows SDK for Windows 7" section of https://github.com/thecodeshop/ruby/wiki/Building-MRI-on-Windows This method uses the same compiler/linker as VS I believe.

I will try the same on `win-file/trunk` and also would like to see someone repro on a different machine.

[Hiroshi Shirosaki]

On Fri, Aug 3, 2012 at 3:53 AM, Luis Lavena <luisl...@gmail.com> wrote:
> On Thu, Aug 2, 2012 at 3:48 PM, Luis Lavena <luisl...@gmail.com> wrote:
>> On Thu, Aug 2, 2012 at 2:55 PM, Jon <jon.f...@gmail.com> wrote:
>>
>>> If it's not yet a feature request, what's left to do before you feel comfortable submitting as a new feature request?
>>>
>>
>> I don't think there is anything else.
>>
>
> Scratch that, if someone can test with Visual Studio build of Ruby,
> will be much appreciated, that way we can confirm it works :)

I tested Visual Studio build, make test, make test-all. That looks fine.
ruby 2.0.0dev (2012-08-01 trunk 36588) [x64-mswin64_100]


--
Hiroshi Shirosaki

[Hiroshi Shirosaki]

>> Scratch that, if someone can test with Visual Studio build of Ruby,
>> will be much appreciated, that way we can confirm it works :)
>
> I tested Visual Studio build, make test, make test-all. That looks fine.
> ruby 2.0.0dev (2012-08-01 trunk 36588) [x64-mswin64_100]
>

I pushed minor fixes for comment style change(// to /* */).


--
Hiroshi Shirosaki

[Luis Lavena]

Thanks, took a diff from ruby:trunk:

https://github.com/thecodeshop/ruby/compare/ruby:trunk...thecodeshop:win-file/trunk.diff

I'll update the patch also to remove the wprintf lines.

I've added my proposal in a gist:

https://gist.github.com/3242245

Please review and comment what you would like to change/add or any
thought you think relevant.

Jon, I agree that we should ask this being backported to 1.9.3,
similar to I/O improvements made by Hiroshi.

I'm planning on submitting the bug this weekend.

[Hiroshi Shirosaki]

I've backported latest patch to win-file/ruby_1_9_3 for next TCS release.

https://github.com/thecodeshop/ruby/tree/win-file/ruby_1_9_3

--
Hiroshi Shirosaki

[Hiroshi Shirosaki]

On Fri, Aug 3, 2012 at 9:03 AM, Luis Lavena <luisl...@gmail.com> wrote:
> I'll update the patch also to remove the wprintf lines.
>
> I've added my proposal in a gist:
>
> https://gist.github.com/3242245
>
> Please review and comment what you would like to change/add or any
> thought you think relevant.
>

Luis, thank you for your work.

If I didn't misunderstand that, the security concerns were pointed out
by Nobuyoshi Nakada. Is this right?
http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/39504

It would be better that benchmark contains practical use case such as
Rails startup time to show big practical advantage.


--
Hiroshi Shirosaki

[Luis Lavena]

On Thu, Aug 2, 2012 at 9:37 PM, Hiroshi Shirosaki <h.shi...@gmail.com> wrote:
> On Fri, Aug 3, 2012 at 9:03 AM, Luis Lavena <luisl...@gmail.com> wrote:
>> I'll update the patch also to remove the wprintf lines.
>>
>> I've added my proposal in a gist:
>>
>> https://gist.github.com/3242245
>>
>> Please review and comment what you would like to change/add or any
>> thought you think relevant.
>>
>
> Luis, thank you for your work.
>
> If I didn't misunderstand that, the security concerns were pointed out
> by Nobuyoshi Nakada. Is this right?
> http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/39504
>

Both, Nobu replied to ruby-core, Usa replied privately.

> It would be better that benchmark contains practical use case such as
> Rails startup time to show big practical advantage.
>

You're right, but I don't remember the mid-size Rails application we
used to bench this before :-(

Will bench an empty app now as starting point.

Thank you.

[Luis Lavena]

On Thu, Aug 2, 2012 at 9:37 PM, Hiroshi Shirosaki <h.shi...@gmail.com> wrote:
>
> It would be better that benchmark contains practical use case such as
> Rails startup time to show big practical advantage.
>

Thanks for the feedback Hiroshi, I've updated the gist this time to
include benchmarks of Rails application startup impact.

PS: the mid-sized Rails application was Enki:

https://github.com/xaviershay/enki

[Jon]

> >>> Scratch that, if someone can test with Visual Studio build of Ruby,
> >>> will be much appreciated, that way we can confirm it works :)
> >>
> >> I tested Visual Studio build, make test, make test-all. That looks fine.
> >> ruby 2.0.0dev (2012-08-01 trunk 36588) [x64-mswin64_100]
> >>
> >
> > I pushed minor fixes for comment style change(// to /* */).
> >
>
> Thanks, took a diff from ruby:trunk:
>
> https://github.com/thecodeshop/ruby/compare/ruby:trunk...thecodeshop:win-file/trunk.diff
>
> I'll update the patch also to remove the wprintf lines.
>
> I've added my proposal in a gist:
>
> https://gist.github.com/3242245
>
> Please review and comment what you would like to change/add or any
> thought you think relevant.
>
> Jon, I agree that we should ask this being backported to 1.9.3,
> similar to I/O improvements made by Hiroshi.
>
> I'm planning on submitting the bug this weekend.

Luis, the gist containing the background and data for your proposal looks good. I have just a couple suggestions:

1) Update "Patch has been tested also against..." to explicitly list OS's tested against, eg - Ubuntu 12.04 GCC 4.X.Y, Win7 32bit MinGW 4.6.2, Win7 64bit MinGW 4.6.1, Win7 64bit Visual Studio ??, OS X Snow Leopard clang ??, etc. Show that the patch works on Windows and also doesn't break *nix.

2) In the "Real life impact: Rails" section, reach out to Alex Chaffey to see if he has additional Rails-on-Windows data. IIRC, he sent an email (ruby-talk ??) saying Rails startup was so slow on Windows (~30s) that it negatively impacted the Ruby/Rails classes he has teaching. It would be spectacular if he'd download the latest tcs-ruby and provide additional Rails startup time data.

Nice work!

[Luis Lavena]

On Fri, Aug 3, 2012 at 7:39 PM, Jon <jon.f...@gmail.com> wrote:
>
> Luis, the gist containing the background and data for your proposal looks good. I have just a couple suggestions:
>

Bug submitted:
http://bugs.ruby-lang.org/issues/6836

> 1) Update "Patch has been tested also against..." to explicitly list OS's tested against, eg - Ubuntu 12.04 GCC 4.X.Y, Win7 32bit MinGW 4.6.2, Win7 64bit MinGW 4.6.1, Win7 64bit Visual Studio ??, OS X Snow Leopard clang ??, etc. Show that the patch works on Windows and also doesn't break *nix.
>

Updated the list of environment on which this was tested.

> 2) In the "Real life impact: Rails" section, reach out to Alex Chaffey to see if he has additional Rails-on-Windows data. IIRC, he sent an email (ruby-talk ??) saying Rails startup was so slow on Windows (~30s) that it negatively impacted the Ruby/Rails classes he has teaching. It would be spectacular if he'd download the latest tcs-ruby and provide additional Rails startup time data.
>

I think having this in front of Core might be better than collect all
the positive feedback we could, simply because the proposal could be
rejected and waiting for the feedback could take longer than a try on
this.

I believe I started to work on Fenix almost a year ago, so I don't
want to delay getting core feedback any longer.

Was planning on mention that the WEBrick fix was bad, take for example
Rack, which also fixes directory traversal without even using
File.expand_path at all.

But didn't want to stir the pot more than necessary, I just want Ruby
be a bit faster :-)

Thank you guys for be there all this time and contributing workloads,
tests results and patches.

Let's see how it goes :-D