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

deprecated: method kills performance #17685

Open
jordanmontt opened this issue Jan 24, 2025 · 2 comments
Open

deprecated: method kills performance #17685

jordanmontt opened this issue Jan 24, 2025 · 2 comments

Comments

@jordanmontt
Copy link
Member

In Pharo, marking methods as deprecated using self deprecated: aMessage significantly slows down method execution.

Let's look some quick benchmarks.

First Fibonacci. The implementation is exactly the same (recursive implementation) the only difference is that the second one adds the self deprecated: 'deprecated' message.

[AClass new fib: 8] bench. "28,388,133 iterations in 5 seconds 3 milliseconds. 5674222.067 per second".

[AClass new fib: 8] bench  "18,406 iterations in 5 seconds 2 milliseconds. 3679.728 per second".

5674222.067 / 3679.728 . "1542.02 times slower"

As we can see from this quick bench, the implementation with self deprecated: is about 1542 (!!) times slower.

Now let's look at a non-recursive method. It's the default factorial implementation from Pharo and as before I just added the self deprecated: 'deprecated'. In this case it's 455 times slower.

[ 8 factorial ] bench "262,071,039 iterations in 5 seconds 3 milliseconds. 52382778.133 per second".

[ 8 factorial ] bench  "575,428 iterations in 5 seconds 1 millisecond. 115062.587 per second".

52382778.133 / 115062.587 "455.25 times slower"

Why?

The slowdown occurs due to the following reasons in the deprecated: method:

deprecated: anExplanationString
	"Warn that the sending method has been deprecated."

	Deprecation new
		context: thisContext sender;
		explanation: anExplanationString;
		signal
  1. Context Reification:
    Each call to the deprecated method creates a new context with thisContext sender

  2. Exception Handling:

Deprecation >> #signal
	| pragma homeMethod |
	homeMethod := context homeMethod.
	(homeMethod hasPragmaNamed: #transform:to:) ifFalse: [ ^super signal ].

	pragma := homeMethod pragmaAt: #transform:to:.
	self rule: pragma arguments first -> pragma arguments second.
	self transform

The method raises an exception to check if it needs to transform. If the transformation is not needed, it triggers the default action, which adds further overhead.

  1. Default Action Processing:
Deprecation >> #defaultAction
	self class addLog: self.
	self showWarning ifTrue: [ self logTranscript ].
	self raiseWarning ifTrue: [ super defaultAction ].
	self shouldTransform ifTrue: [ self transform ]

The default action checks system settings to determine if a warning should be raised. If deprecations are turned off (which many users do), this processing still occurs without any benefit, leading to wasted resources.

TLDR

So, in summary, if the user turned off the deprecations from the system settings (like most of the people do), all of this work is done for free. There is the hidden exception raised and the context reification that will pass completely unnoticed; killing performance.

@MarcusDenker
Copy link
Member

We should check if we can move the preference checking ealier.

But this reminds me of:

I once read a blog post where instead of deprecating, they just made a method slower with every release.

People started to refactor to new API for speed, without realizing they were "tricked" into cleaning...

@Ducasse
Copy link
Member

Ducasse commented Jan 26, 2025

Hi seb

Thanks this is nice. Could you publish it as a blog post? because like that people should move faster out of deprecated methods :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants