Ticket #1925 (closed RFC: fixed)

Opened 4 years ago

Last modified 4 years ago

remove config step auto::jit

Reported by: coke Owned by: jkeenan
Priority: normal Milestone:
Component: configure Version: 2.11.0
Severity: medium Keywords:
Cc: cotto, whiteknight Language:
Patch status: new Platform:

Description

Looks like it hardcodes some values in the config, but doesn't do
much. Since we don't currently have a JIT, this config step can
probably be removed without any harm.

-- 
Will "Coke" Coleda

Attachments

90418f5c8cbb8.remove_auto_jit.diff Download (8.8 KB) - added by jkeenan 4 years ago.
diff of remove_auto_jit branch against its branch point

Change History

  Changed 4 years ago by coke

This message has 0 attachment(s)

follow-up: ↓ 3   Changed 4 years ago by jkeenan

  • type set to bug
  • component changed from none to configure
  • milestone 2.11 deleted

IIRC, when we ripped out the old, dysfunctional JIT, we considered removing this config step (and its associated tests as well). But some people felt we'd be getting the new JIT before long, so there was no harm in leaving it in.

Well, "before long" hasn't arrived yet, so I have no objection to proceeding with removal.

But perhaps we could get a GCI task out of this:

Evaluate the impact of removing configuration step auto::jit.

Task would be to (a) identify the impact of removing any of the values currently set by this configuration step -- hopefully demonstrating that their removal does no harm; (b) actually remove the config step, its tests, etc.

How does that sound?

kid51

in reply to: ↑ 2   Changed 4 years ago by jkeenan

  • status changed from new to assigned
  • owner set to jkeenan
  • type changed from bug to RFC

But perhaps we could get a GCI task out of this:

See  this published GCI task.

Changed 4 years ago by jkeenan

diff of remove_auto_jit branch against its branch point

follow-up: ↓ 5   Changed 4 years ago by jkeenan

  • cc cotto, whiteknight added
  • patch set to new

Created the remove_auto_jit to work on this ticket. Removed the configuration step, its associated test, associated mentions in documentation, options lists, etc.

I had to touch more files than I would have expected, largely because there are instances of EXEC_CAPABLE and JIT_CAPABLE in quite a few places in the code.

Please review the attached patch, which passed make test and make fulltest.

Thank you very much.

kid51

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 4 years ago by jkeenan

Replying to jkeenan:

Please review the attached patch, which passed make test and make fulltest.

On review, it may not have passed make fulltest. It may have failed make benchmark_tests. I'll investigate, but this shouldn't stop others from reviewing the patch.

kid51

in reply to: ↑ 5   Changed 4 years ago by jkeenan

Replying to jkeenan:

On review, it may not have passed make fulltest. It may have failed make benchmark_tests. I'll investigate, but this shouldn't stop others from reviewing the patch.

Never mind; couldn't replicate the failure.

  Changed 4 years ago by jkeenan

Per discussion with cotto, merged this branch into trunk. The merge was messy, but everything should be okay now.

We noted that we still have t/op/jit.t and t/op/jitn.t is the distribution. However plobsing points out that "they were conceived as basic execution tests for the JIT, but they are applicable to anything that executes ops". So we'll leave them be.

Will keep ticket open for 2-3 days for comments, complaints.

Thank you very much.

kid51

  Changed 4 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

No complaints; closing ticket.

Note: See TracTickets for help on using tickets.