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

Eagerly require date #695

Merged
merged 1 commit into from
Nov 20, 2024
Merged

Eagerly require date #695

merged 1 commit into from
Nov 20, 2024

Conversation

tdeo
Copy link
Contributor

@tdeo tdeo commented Nov 13, 2024

Fixes #694.

When picking up only the test changes (and not require 'date' explicitly anymore), such failures start to appear:

Error: test_spec_sequence_key_shortcut(Psych_Unit_Tests): NameError: uninitialized constant Psych_Unit_Tests::Date
/Users/thierry/workspace_personal/psych/test/psych/test_yaml.rb:299:in `test_spec_sequence_key_shortcut'
     296:     def test_spec_sequence_key_shortcut
     297:         # Shortcut sequence map
     298:         assert_parse_only(
  => 299:           { 'invoice' => 34843, 'date' => Date.new( 2001, 1, 23 ),
     300:             'bill-to' => 'Chris Dumars', 'product' =>
     301:             [ { 'item' => 'Super Hoop', 'quantity' => 1 },
     302:               { 'item' => 'Basketball', 'quantity' => 4 },
=======================================================================================================================
Finished in 0.390079 seconds.
-----------------------------------------------------------------------------------------------------------------------
609 tests, 1481 assertions, 0 failures, 21 errors, 0 pendings, 0 omissions, 0 notifications
96.5517% passed

The autoload :Date statement gets moved to a more appropriate file (the one actually loading the constant), and moved to the top-level so it requires ::Date and not try to require Psych::ClassLoader::Date

Performance-wise, reusing the benchmark from 06db36f#diff-6a459e056cadf37665f54005bd2dde09d9ba8e66c9807eb0dc67145f9b841771R7, it seems to further improve performance quite unsignificantly:

With those changes:

$ ruby /tmp/bench-yaml.rb
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [arm64-darwin23]
Warming up --------------------------------------
           100 dates   464.000 i/100ms
Calculating -------------------------------------
           100 dates      4.605k (± 2.3%) i/s  (217.18 μs/i) -     23.200k in   5.041382s

On current master:

$ ruby /tmp/bench-yaml.rb
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [arm64-darwin23]
Warming up --------------------------------------
           100 dates   439.000 i/100ms
Calculating -------------------------------------
           100 dates      4.515k (± 1.9%) i/s  (221.49 μs/i) -     22.828k in   5.058050s

@tdeo tdeo mentioned this pull request Nov 18, 2024
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any test, so it's hard to confirm it's indeed the cause.

@@ -2,6 +2,8 @@
require_relative 'omap'
require_relative 'set'

autoload :Date, "date"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should set consts on Object like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you rather change it to a require 'date' statement at the top-level?

@tdeo
Copy link
Contributor Author

tdeo commented Nov 19, 2024

I don't see any test, so it's hard to confirm it's indeed the cause.

I tried to write some tests but unfortunately Kernel.remove_const(:Date) is impossible because Date is a built-in. I meant in the PR description that if you try to run the test suite without the autoload statement, you'll get a failure.
You would also get failures if that statement is within Psych::ScalarScanner, in which case it's trying to autoload Psych::ScalarScanner::Date instead of ::Date

@byroot
Copy link
Member

byroot commented Nov 19, 2024

You can probably use a subprocess.

@tdeo
Copy link
Contributor Author

tdeo commented Nov 19, 2024

You can probably use a subprocess.

I was able indeed to add a test case with this technique, thanks for the suggestion

@byroot
Copy link
Member

byroot commented Nov 19, 2024

There's a test helper for that: assert_separatly.

But no worries, I'll take care of it. I need to run some errand but I'll fix this in a couple hours.

Also I think we should just stop lazy requiring libraries like that, it's stupid.

@@ -223,6 +223,13 @@ def test_string_with_ivars
assert_equal ivar, food.instance_variable_get(:@we_built_this_city)
end

def test_string_matching_date
assert_separately(%w[-r ./lib/psych], <<~RUBY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_separately(%w[-r ./lib/psych], <<~RUBY)
assert_separately(%w[-I lib -r psych], <<~RUBY)

I think you need to pass -I otherwise it risks loading the extension from stdlib.

@@ -1,6 +1,7 @@
# frozen_string_literal: true
require_relative 'omap'
require_relative 'set'
require 'date'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should declare date in the gemspec?

@byroot byroot force-pushed the tdeo/fix_date_require branch from a50d82e to 3de6c27 Compare November 19, 2024 11:09
@byroot byroot changed the title Autoload date properly when needed Eagerly require date Nov 19, 2024
@byroot
Copy link
Member

byroot commented Nov 19, 2024

I updated your PR.

cc @tenderlove what do you think?

@byroot byroot force-pushed the tdeo/fix_date_require branch from 3de6c27 to b2aa003 Compare November 19, 2024 11:22
@@ -75,6 +75,8 @@ DESCRIPTION
s.add_dependency 'stringio'
end

s.add_dependency 'date'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that make sense for the stdlib?

@tdeo
Copy link
Contributor Author

tdeo commented Nov 19, 2024

Thanks for wrapping it up

Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this? The change seems fine, I'm just not following why.

@byroot
Copy link
Member

byroot commented Nov 19, 2024

Right, sorry Aaron, I should have explained.

In YAMLTree#visit_String, there's this condition to figure if a string should be quoted:

        elsif not String === @ss.tokenize(o) or /\A0[0-7]*[89]/.match?(o)
          style = Nodes::Scalar::SINGLE_QUOTED

So we end up in:

class_loader.date.strptime(string, '%F', Date::GREGORIAN)

Which further end up trying ClassLoader#resolve of "Date", and this uses path2class, so it's resolved as essentially ::Date, so the scoped autoload I introduced isn't triggered in this case.

Hence why the initial version of this PR was to move the autoload at the top level, but at that point I think we should just eager load.

@tenderlove
Copy link
Member

@byroot ah, gotcha. I somewhat worry this might have downstream effects (maybe on Bundler or RubyGems). But I like the change so I'll merge it.

@tenderlove tenderlove merged commit a1ce01c into ruby:master Nov 20, 2024
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Breaking change in 5.2.0
3 participants