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

Update apply-reduction-relation* to be sensitive to alpha-equivalence #102

Closed
wants to merge 1 commit into from

Conversation

howell
Copy link
Contributor

@howell howell commented May 25, 2017

Also add an immutable alpha hash constructor and update hashes in
the gui to use the form provided by binding-forms.rkt

Fixes #96

Also add an immutable alpha hash constructor and update hashes in
the gui to use the form provided by binding-forms.rkt

Fixes racket#96
@wilbowma
Copy link
Collaborator

I just ran the microbenchmarks in redex-test/redex/tests/binding-performance-test.rkt with this branch. The eval tests seem to run a fair bit slower. I also tried replacing the judgment form cache with make-alpha-hash, and experienced similar slow downs.

@wilbowma
Copy link
Collaborator

For reference, measurements here: https://gist.github.com/wilbowma/99bcbc4c97f2b970091b9f43754782b2

@wilbowma
Copy link
Collaborator

I pushed a fork of this branch to https://github.com/wilbowma/redex/tree/alpha-caches, which includes using an alpha hash for judgment forms. The judgment form slow downs are in fact much more significant. Some factors over 10x

@rfindler
Copy link
Member

Are you measuring this slowdown on languages that have binding forms specified or on ones that don't?

@wilbowma
Copy link
Collaborator

.. That's a good question. I'll have to take a look at the tests.

@wilbowma
Copy link
Collaborator

Both with and without judgment forms are slower. If you look at the logs I posted in the above gist, compare the last test in baseline.log and judgment-cache.log. complex-ho is run in both stlc+lists with and without binding forms. Both are significantly slower.

@rfindler
Copy link
Member

rfindler commented May 27, 2017 via email

@wilbowma
Copy link
Collaborator

Eek. I was hoping this would help performance. I've got some pathological cases where variables get freshened tens of thousands of times, and they seem to defeat the cache.

@rfindler
Copy link
Member

rfindler commented May 27, 2017 via email

@wilbowma
Copy link
Collaborator

I'll try to get you some code later. Will my CIC model do? I know I can reproduce the behavior there, but it's awfully complex.

@rfindler
Copy link
Member

rfindler commented May 27, 2017 via email

@wilbowma
Copy link
Collaborator

I'm going to move this discussion to a separate PR. I tested @howell's PR against my extensive development and do get performance improvements (nearly 2x!). Maybe my changes to judgments are broken.

@wilbowma
Copy link
Collaborator

wilbowma commented Jun 2, 2017

@howell, looking over this PR, it appears that some caches are still hashes. E.g., line 1708 in reduction-semantics.rkt, and line 1719

@wilbowma
Copy link
Collaborator

wilbowma commented Jun 2, 2017

Looks like those are the metafunction caches, and performance dies similar to judgment forms when I change them.

@@ -137,6 +137,11 @@ to traverse the whole value at once, rather than one binding form at a time.
(λ (x) (α-equal-hash-code language-bf-table match-pattern x))
(λ (x) (α-equal-secondary-hash-code language-bf-table match-pattern x))))

(define (make-immutable-α-hash language-bf-table match-pattern)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the make-*-custom-hash interface is deprecated, perhaps this is a good time to redefine these using define-custom-hash-types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I missed that these were deprecated.

The define-custom-hash-types interface is a bit awkward for this use case. The functions for computing equality and hashing need to close over the language binding table and match pattern, which aren't provided until we actually make a hash. So the result would look like

(define (make-α-hash language-bf-table match-pattern)
  (define-custom-hash-types this-lang-α-hash
    (λ (x y) (α-equal? language-bf-table match-pattern x y))
    (λ (x) (α-equal-hash-code language-bf-table match-pattern x))
    (λ (x) (α-equal-secondary-hash-code language-bf-table match-pattern x)))
  (make-mutable-this-lang-α-hash))

It's a bit awkward, but maybe still better than using a deprecated version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Except it will be worse, since define-custom-hash-types defines 7 names. You need to abstract all 7 definitions over the bf-table and match-pattern.. it's a bit gross.
Makes me want a macro that is like splicing-let, but instead of splicing in definitions, it .. splices in additional arguments? Essentially, allows eta expanding a bunch of definitions with respect to a set of arguments ..?

(splicing-lambda (x y)
  (define-values (f g h) (values (lambda (z) x) (lambda (z) y) (lambda (z) z))))
(check-equal? (f 1 2 3) 1)
(check-equal? (g 1 2 3) 2)
(check-equal? (h 1 2 3) 3)

@wilbowma
Copy link
Collaborator

wilbowma commented Aug 9, 2018

  1. I found a bug in this; see https://github.com/wilbowma/redex/tree/pr-102
  2. Can we merge this? I'm running into massive performance problems again and had to manually install this patch.

@wilbowma
Copy link
Collaborator

wilbowma commented Aug 9, 2018

(By the way, the bug I found suggests there are no tests for apply-reduction-relation* using an IO judgment form in the test suite)

@rfindler
Copy link
Member

rfindler commented Aug 9, 2018

In tl-judgment-form.rkt, these are tests of IO-judgment-forms being passed to apply-reduction-relation, line 518:

(let ()
  (define-judgment-form empty-language
    #:mode (J O I)
    [------------- "smaller"
     (J any (any))]
    
    [----------------- "bigger"
     (J ((any any)) any)])
  
  (test (judgment-form? J) #t)
  (test (IO-judgment-form? J) #t)
  (test (apply-reduction-relation J '(2))
        (judgment-holds (J any (2)) any))
  (test (apply set (apply-reduction-relation/tag-with-names J '(3)))
        (set (list "smaller" 3)
             (list "bigger" '(((3) (3)))))))

@rfindler
Copy link
Member

rfindler commented Aug 9, 2018

Rereading the conversation I'm not really clear what is going on here. Looking at the commit diff, it seems like the changes are good ones, however, so I'll go ahead and merge it. If there is something else that needs doing, please let me know.

@wilbowma
Copy link
Collaborator

wilbowma commented Aug 9, 2018

Well, the early conversion was a bit of topic and moved to #103.
This PR, except for the bug I fixed in https://github.com/wilbowma/redex/tree/pr-102, looks good to me, and really helps performance on some of my models. There's one minor issue: it's using a deprecated inference for creating a custom hash.

@rfindler
Copy link
Member

rfindler commented Aug 9, 2018

That part seems to already be in redex. I'll work thru the diff and push what was clearly intended.

@rfindler rfindler closed this in 35da193 Aug 9, 2018
@rfindler
Copy link
Member

rfindler commented Aug 9, 2018

Thanks for bringing this back to my attention. I took the liberty of putting your name on the revised commit, @howell .

@wilbowma
Copy link
Collaborator

Huzzah! Thanks!

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

Successfully merging this pull request may close these issues.

3 participants