Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

x64対応を何をもって対応と言うか。 #430

Closed
3 tasks
KENCHjp opened this issue Sep 9, 2018 · 31 comments
Closed
3 tasks

x64対応を何をもって対応と言うか。 #430

KENCHjp opened this issue Sep 9, 2018 · 31 comments
Labels
management 運営に関する話題 【ChangeLog除外】 Release Release作業チケット【ChangeLog除外】 x64 x64 対応

Comments

@KENCHjp
Copy link
Member

KENCHjp commented Sep 9, 2018

Next Releaseに向けて目玉の一つがx64対応かと思いますが、
現在alphaリリースのものを正式リリースと言うために何をしたらいいかです。

  • ソース対応(完了?)
  • UnitTest(実施済?)
  • 固有機能実装(何かx64で実装しないといけないものがありますか?)
@KENCHjp KENCHjp added the Release Release作業チケット【ChangeLog除外】 label Sep 9, 2018
@berryzplus
Copy link
Contributor

berryzplus commented Sep 9, 2018

現在alphaリリースのものを正式リリースと言うために何をしたらいいかです。

一番分かりやすいのは 32bit アプリじゃ無理だと思われることをやれればいいんだと思います。
具体的には、4GBのファイルをサクサク編集できるようにする、とかです。
これができたら win10 対応終わってなくてもリリースする価値があります。
「松・竹・梅」の「松」プランなので、ここを目指す必然はないです。

サクラエディタの内部データでテキストのサイズを示す変数の型は、ほぼすべてintです。
int型は32bitの符号付き整数なので、2GBまでのサイズしか扱えません。
これをなんとかして ptrdiff_t 型に置換すること、それができれば 64bit 版のビルドで 4GB を超えるデータを扱えるようになります。

サクラエディタの内部には、そもそも64bitでビルドすることを想定してないコードが結構あります。
そういうコードを 64bit 向けにビルドすると、警告がいっぱい出ます。
それらの警告をすべて潰(=出ないように直す)せたら x64 対応と言える気がします。
「松・竹・梅」の「梅」プラン、最低クリア条件だと思います。

上を目指すのは「梅」にたどり着く目途がついてからでいいのかな、とぼくは思ってます。
どこまでやります?です。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 9, 2018

なるほど、「松」「梅」表現いいですね。
リリースというからには「松」というか、
4GBのファイル開いて編集するテストなら私での出来るので「梅」ができたらalphaリリースして広くテスター募りましょうかね。
ちなみに「梅」は、 #206 ですよね。
これは対応するの遠いのかな。

今リリースされているx64はとりあえずコンパイルできた、って状態で4GBファイルは開けないってことですかね(なんかそのまえに2GB Overチェック入ってるんでしたっけ)

@m-tmatma m-tmatma added the x64 x64 対応 label Sep 10, 2018
@KENCHjp KENCHjp added the Release Release作業チケット【ChangeLog除外】 label Sep 18, 2018
@m-tmatma
Copy link
Member

x64 対応に関して誰か作業されてますか?

@kobake
Copy link
Member

kobake commented Sep 18, 2018

先週末に一気に片付けようと着手しましたが体調思わしくなく頓挫しました。今週末にでもチャチャっと対応しちゃおうと思ってますが、自分より先に対応できちゃう方がいるのであればその方に任せても良いです。

@kobake
Copy link
Member

kobake commented Sep 18, 2018

自分のほうで push しているブランチも無いですし、他の方が作業着手することを止めはしません。

@kobake
Copy link
Member

kobake commented Sep 18, 2018

ちなみに自分が片付けようとしたのは全Warningの解決です。これでx64完了というわけではないですが、ひとつのステップとしてキリは良いでしょう。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

今、だれも着手してないかもしれませんが、ひとまず全Warning消せて、4GB overのファイル開いて疎通確認したら、alphaをbetaにして、広くテストしてもらうのどうでしょう。

@berryzplus
Copy link
Contributor

前に @m-tmatma さんが部分的なPRを出されていたと思います。
あんな感じに小分けにしてエラー番号ごとの対応を行っていったらいいのではないかと思います。

当時はどういう風にx64化するかの指針が何もなくて、
x64前提側に倒す修正になってたので「待った」をかけたはずです。

こういう意味のエラーだから、
こんな感じに対処します。

と宣言して、対応していく感じになるのかな、と。

その際、x64化のために既存の構造を拡張したり改善したりしない、って感じがスムーズに進めてくための指針になるかと思っています。

この件に関して、ぼくはレビュー側に回ったほうがよさそうな気がしています。
レビューしたい!って人がいればぼくがPR出しますけど 😢

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

@berryzplus さん、反応感謝です^^;

ソース見てない身なので小出しでいけるのか一括で変更しないとだめなのか、
既存ロジックへの影響は避けられるのかなど全然わかってません。

だれか修正のパターン、こういう時はこうしましょうとか、
事例があればみんなでソース分担して一斉にとりかかれるのかな。。。
いよいよ本腰で援軍募ろうかなと。

@berryzplus
Copy link
Contributor

警告一覧は Excel ファイルになっていて作業分担はできる状態なんですよね。

https://ci.appveyor.com/project/sakuraeditor/sakura/branch/master/job/p3ul39art0458p1w/artifacts
log.zipの中に入ってます。

ざーーーーーっと見たところ、ほぼ「 64bit を 32bit に暗黙変換してますけど大丈夫ですか?」という意味の警告っぽいです。

第一段階としては「いいです。」ということになるんだと思ってます。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

警告一覧は Excel ファイルになっていて作業分担はできる状態なんですよね。

なるほど。とはいえ、割り振るほどヒューマンリソース潤沢じゃないので、
余力とモチの有る人がちょっとづつ宣言して着手って感じですかね。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

第一段階としては「いいです。」ということになるんだと思ってます。

64Bitにせねばならないところとそうでないところを分けるって感じですかね。

@berryzplus
Copy link
Contributor

第一段階としては「いいです。」ということになるんだと思ってます。
64Bitにせねばならないところとそうでないところを分けるって感じですかね。

いや、いったん警告潰すだけっす。
機能的には何も変わらないですが、それをやる価値はあると思っています。

x64 になったら何を拡張できるか考えるのはその後です。

拡張できたらうれしい部分は多いですが、
拡張しても意味無いところもあるので、そこは考えないといかんと思ってます。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

いや、いったん警告潰すだけっす。

そうなんっすね、わかってない中いってますが、64Bit化して4GB Over対応のところを見つけるのが今がいいのかなと思ったのですが、一旦フラグ消してからでもいいって感じなんすね。

@m-tmatma
Copy link
Member

第一段階としては「いいです。」ということになるんだと思ってます。
64Bitにせねばならないところとそうでないところを分けるって感じですかね。

いや、いったん警告潰すだけっす。

これは何を意味してますか?

64Bit 環境の場合に 64Bit 変数が必要か判断して適切な型を使うという意味ですか?
単に警告を出さないように押さえてしまうだけならそのままの方がいいと思います。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

単に警告を出さないように押さえてしまうだけならそのままの方がいいと思います。

実装レベルは想像の域をでませんが、私の空想は @m-tmatma さんの思いに近いとおもってます。
どうでもいいところは早々に潰して、でかい値を扱うところはでかくするみたいな。

@berryzplus
Copy link
Contributor

お二人とも正しいと思います。

発生している警告の種類は少し前に書いたとおりです。
対策するにはキャストを書くことになります。
対応が終わったあとのコードがどうなるか想像してみてください・・・

たぶん、よくわからない(int)でいっぱいになってると思います。

実際のところ、x64対応は警告潰しなんかじゃなくて、何をx64対応させないといけないかを見定めることだと思っています。そのために、現状の警告はノイズになります。取り払っておくに越したことはないと思います。

@kobake
Copy link
Member

kobake commented Sep 21, 2018

すみません、離れると言いましたが一応通知は見ているのでここだけは突っ込ませてください。
安易なキャストによる警告潰しは絶対にダメです。
これらの警告はノイズではなく対応すべき場所を示す道標です。せっかくの道標を潰してはダメです。

僕は正直、完璧主義は嫌いです。なのでPRマージ等も今よりもっと雑にやって良いと思っています。

ただ、この件に関しては雑にやると本気で大怪我するので気を付けてください。
雑対応大好きな僕が見逃せないくらいやばい対応です。キャストで警告潰すのは。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

たぶん、よくわからない(int)でいっぱいになってると思います。

intのキャストって動的なんすかね。
わたしはプリプロセッサレベルで静的にコーディング分岐するイメージでおりました。
極論するとForkして別物レベル。

が、

実際に手つけると意外と局所化できるかもしれないっすよね。
こういうの得意な人いるのかなぁ。。

@kobake
Copy link
Member

kobake commented Sep 21, 2018

一応補足しておくとビット数または値範囲が分かっている変数同士をどうしても橋渡ししなければならない都合で、キャスト(または #pragma 等)によってしか潰せない警告をキャストで潰す対応が必要なことが多々あることもまた事実です。ただ x64 対応については真摯にサイズ問題と向き合わなければならない分野です。

@kobake
Copy link
Member

kobake commented Sep 21, 2018

実際のところ、何をもって x64 が完了したかについては、x64 対応による影響範囲を洗い出し、そのテストを行い、それがすべて正常に動くことを確認する必要があります。

ただ、その影響範囲は x64 関連の警告を潰すことによって徐々に発掘されることになるはずです。なので対応以前に影響範囲を全て洗い出すことはおそらく不可能です。

x64 関連の警告を潰していくと、その変更コード範囲から影響を受ける機能が洗い出せるので、それをもとにテスト計画を立てる、というのが真っ当な筋道です。これなんか仕事っぽくて嫌ですが、x64 に限っては仕方ないです。

普通の機能対応だったら対応前から先にテスト計画を立てられるのが一般的ですが、x64 についてはちょっと逆向きの方向でものを考える必要があります。そのためにもひとつひとつの警告を丁寧に扱ってあげてください。

@m-tmatma
Copy link
Member

単に警告を出さないように押さえてしまうだけならそのままの方がいいと思います。

勘違いされているかもしれませんが、補足すると、私は @kobake さん派です。

キャストをして警告を握りつぶすのを選ぶくらいなら、何も変えずに
そのまま警告が大量に出ている状態を維持して何も変えないというという意味です。

警告を握りつぶすのは、害しかないです。

実際のところ、x64対応は警告潰しなんかじゃなくて、何をx64対応させないといけないかを見定めることだと思っています。そのために、現状の警告はノイズになります。取り払っておくに越したことはないと思います。

警告を修正することには賛成です。

しかし、64bit OS では 64bit 変数にすべき(or した方がいい) ものを64bit 変数にして
32bit OS で 32bit 変数でいいところを 32bit 変数にする方向で対応する必要があります。

@berryzplus
Copy link
Contributor

それだとメチャメチャ時間がかかるからモチベがもたない・・・というのを懸念しています。

芋づる方式というか、指摘された箇所で「何故警告が出たのか」を考えて原因に対処していくやり方になると思います。原因に対処すると警告箇所以外にもあちこち修正することになります。

この方式だとPRする側もレビューする側も、かなりガチンコで取り組むことになります。
どれくらいガチンコかというと、所見ではコメントすら書かないくらいの真剣度だと思います。
GitHubに移行してまだ日も浅い今、それができるかどうかぼくには疑問です。

それでも「ガチンコで行こう」っていうなら付き合うのもやぶさかではないですが。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 21, 2018

それだとメチャメチャ時間がかかるからモチベがもたない・・・というのを懸念しています。

同じく懸念はしてます、はい。
でも、単にフラグを消しただけだとおそらく4GBのファイルは開けないですよね、たぶん。
フラグを消しただけだと、中の人の満足でしかないのかなと。
いや、えっと @berryzplus さんにダメ出ししたいわけじゃなく、ゴールはどこでそれにむかってどうするかってことかと。

そしてこの修正ってトリッキーな実装ではなく地道で堅実な人が好きそうな分野なのかなと。
新しい機能実装するわけじゃないからしんどいだけですし。

なので援軍呼ぼうかなぁって。

@berryzplus
Copy link
Contributor

64bit化しないといけないint

  • メモリサイズ
  • OSに渡すハンドル
  • システム系のカウンタ(タイマーとか)
  • メモリアドレス(通常はポインタだから考慮しなくていいはず)

64bit化しなくてもいいint

  • 小規模ループのループカウンタ for (int i = 0; i < MAX_NANIKA; i++)
  • UNICODEコードポイント 20bitだから
  • 桁数とか行数とか配列インデックスとか たぶん16bitで十分足りる

 

@berryzplus
Copy link
Contributor

この2つだけ対応すればいけるんじゃね?というのが何度か試行してみた感想です。
他にもあるかも知れませんが、ぼくが想像できる限りではここを対処すれば64bitっぽくなります。

  • メモリサイズ
  • システム系のカウンタ(タイマーとか)

OSに渡すハンドルを対応しなくてよいと考えている理由は、多くの部分で Windows SDK がx86/x86_64の違いを吸収してくれているためです。

x64対応を検討する中で思いついた性能強化案がいくつかあります。
それをどう扱うべきか自分の中で結論を出てなかったんですが、
ちょっと出して行こうかなと思っています。

警告を見て思いついたものなので、いま出すと確実に修正範囲が被ります。
あとで出すと対応内容を上書くので「あの苦労は一体・・・」が懸念されます。

やるかやらんかは別にして、issueを立てる障害にはならんと思うので。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 22, 2018

とりあえず、気が早いかもですが、5Gのテキストファイルは作りました!

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 22, 2018

32bitではエラーになりましたが、64bitだと開けました。
default
ただ私のPCだとHDDネックで超遅い・・・

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 22, 2018

正確に5GBじゃなかったので、リトライ中。

@KENCHjp
Copy link
Member Author

KENCHjp commented Sep 22, 2018

とりあえず5GB開けたので末尾修正して保存したら、
default
1GB程度のファイルになっちゃいましたT_T
2

@berryzplus
Copy link
Contributor

スモークテスト(#1301)で問題なければ「できた!」と言ってよい気がします。
なにをもってスモークテスト(≒全機能網羅)と言えるかは別件だと思いますが。

というわけで閉じてしまいます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
management 運営に関する話題 【ChangeLog除外】 Release Release作業チケット【ChangeLog除外】 x64 x64 対応
Projects
None yet
Development

No branches or pull requests

5 participants